Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Binding LLVM itself #172

Open
makslevental opened this issue Feb 22, 2022 · 14 comments
Open

Binding LLVM itself #172

makslevental opened this issue Feb 22, 2022 · 14 comments

Comments

@makslevental
Copy link

Thanks for building this - it's very useful. One question: have you tried to use binder to bind to LLVM itself? I've tried and run into forward decl issues (ie binder only finds forward decls and not the actual impls that are buried in various .inc files).

@lyskov
Copy link
Member

lyskov commented Feb 24, 2022

I have not tried binding LLVM itself but it sounds like an interesting project! - on issue itself: it sounds like Binder is not processing .inc files, - is that right? If so, could you please double check that you added these files into your project all-includes.hpp file?

@makslevental
Copy link
Author

makslevental commented Feb 24, 2022

here is my all-includes.hpp:

#include <circt/Dialect/Calyx/CalyxDialect.h>

(lol)

The header is actually part of the CIRCT project (which sits on top of LLVM and uses much of the same infrastructure). It contains includes for the .inc files.

Here is my script that runs binder

/home/mlevental/dev_projects/binder/build/bin/binder --root-module my_project --prefix \
 /home/mlevental/dev_projects/circt/hls/bindings \
  --bind "circt" all_includes.hpp \
  -v \
   -- -std=c++17 \
    -I/home/mlevental/dev_projects/circt/cmake-build-debug-llvm/include \
    -I/home/mlevental/dev_projects/circt/cmake-build-debug-circt/include \
    -I/home/mlevental/dev_projects/circt/cmake-build-debug-llvm/tools/mlir/include \
    -I/home/mlevental/dev_projects/circt/llvm/llvm/include \
    -I/home/mlevental/dev_projects/circt/llvm/mlir/include \
    -I/home/mlevental/dev_projects/circt/include \
     -DMY_PROJECT_DEFINE -DNDEBUG

I have instrumented binder (mostly putting back in your debug prints) to try to debug - print log is here.

The final error generated by binder is

ERROR: Could not find binder for type: mlir::StorageUniquer::BaseStorage!
Usually cause for this is that type was only forward declared. Please check that Binder input include file have full declaration of this class.

which if you consult the log file, is actually successfully "discovered" and "is_bindable":

is_bindable(CXXRecordDecl): mlir::StorageUniquer::BaseStorage C->hasDefinition():1 C->isCompleteDefinition():1 C->isThisDeclarationADefinition():1 C->getDefinition():0x55ab85eecbb0 C->isDependentType():0

If you'd like I can put together a MWE somewhere but I'm sure if you try to bind anything in LLVM itself you'll probably get similar failure modes.

@lyskov
Copy link
Member

lyskov commented Feb 25, 2022

@makslevental all-include.hpp should absolutely have include files for all types that you planning to bind. Just includes with forward declarations will not do: - full declarations will be needed. Hope this helps,

@makslevental
Copy link
Author

...i think you understand that for LLVM that will end up being every single header in the build directory? Is this really a limitation of clangast (it can't parse headers to find more headers) or are you imposing this limitation? Also this won't work even if you try (I've tried) because .inc files have include guards - you will absolutely see a "twice included" or something like that error.

@lyskov
Copy link
Member

lyskov commented Mar 3, 2022

@makslevental I am not sure if fully understand you question but I guess we can say that this is limitation of whole technology in general: you see, - in order to generate bindings we have to know full information about classes. Particularly we have to know each member function type signature. We do not need however to know implementation of the functions includes with such information could be omitted.

To summarize: in order to generate Python bindings Binder have to parse full class definition of C++ classes, - there is no way around it.

@makslevental
Copy link
Author

@lyskov I understand that you need full class definitions. I'm asking whether you really need to include all of the headers by hand in all-include.hpp since this is what you said:

all-include.hpp should absolutely have include files for all types that you planning to bind.

So I'm asking whether clangast can discover #includes inside #includes. I'm pretty sure the answer is in fact yes, in which case I don't understand your response there (i.e., that I need to have all the necessary #includes in all-include.hpp).

@lyskov
Copy link
Member

lyskov commented Mar 3, 2022

oh, i see what you asking, let me rephrase my answer: parsing all-include.hpp should give compiler full definition of classes that we going to bind. This could be archived by including files describing classes directly or indirectly (ie by inducing files that will include other files etc).

@makslevental does this answer your question?

@makslevental
Copy link
Author

@lyskov Yes that clears up your comment but still leads me to be confused about why this is failing in this case? Can you suggest maybe something I could try to try to debug? Like I said I've already tried uncommenting some of your debug statements but I still couldn't quite figure out what the failure mode was.

@makslevental
Copy link
Author

makslevental commented Mar 3, 2022

@lyskov Here's a reproducible failure assuming you have LLVM somewhere (should be a safe assumption 🤣):

make_binder.sh

/home/mlevental/dev_projects/binder/build/bin/binder --root-module my_project --prefix \
 /home/mlevental/dev_projects/circt/hls/bindings \
  --bind "mlir" all_includes.hpp \
  -v \
   -- -std=c++17 \
    -I/home/mlevental/dev_projects/circt/cmake-build-debug-llvm/include \
    -I/home/mlevental/dev_projects/circt/cmake-build-debug-llvm/tools/mlir/include \
    -I/home/mlevental/dev_projects/circt/llvm/llvm/include \
    -I/home/mlevental/dev_projects/circt/llvm/mlir/include \
    -I/home/mlevental/dev_projects/circt/include \
     -DMY_PROJECT_DEFINE -DNDEBUG

all_includes.hpp

#include <mlir/Dialect/ControlFlow/IR/ControlFlowOps.h>

@lyskov
Copy link
Member

lyskov commented Mar 3, 2022

ok, i just re-read whole conversation and i think i understand a bit better whats going on. Let me summarize:

  • if you run Binder without modification then you will get forward declaration error issue
  • (a) if you modify Binder then you will get some debug output
    For future runs please only use unmodified version of Binder since (a) output just create confusion here.

In short: as i mention include file should contain all declarations. If you are getting forward declaration error this will be from LLVM parser level (ie before Binder have chance to work on it). And that should be addressed first. I see that your includes actually do have .imp includes though. Have you checked the include files in question? Does they actually contain output that you need?

Also, from the log file that you posted it is unclear what errors you run into (if any). Could you please re-run this with unmodified version of Binder and post log file? (if you going to share using GDrive please make sure to use .txt extension so it is possible to view files without downloading them - thanks,)

@makslevental
Copy link
Author

makslevental commented Mar 10, 2022

I'm not sure how to explain more clearly what's happening (since I posted a MWE). So I'll try again:

Here is the error that unmodified (master tip) binder produces:

Generate bindings, pass 1...
Generate bindings, pass 2...
Generate bindings, pass 3...
Generate bindings, pass 4...
Generate bindings, pass 5...
Sorting Binders...
ERROR: Could not find binder for type: mlir::MemRefLayoutAttrInterface::Trait<mlir::AffineMapAttr>!
Usually cause for this is that type was only forward declared. Please check that Binder input include file have full declaration of this class.

Here is a test.cpp that compiles fine with my regular build system (CMake)

#include <mlir/IR/BuiltinAttributeInterfaces.h>
#include <mlir/IR/BuiltinAttributes.h>

int main() {
  mlir::MemRefLayoutAttrInterface::Trait<mlir::AffineMapAttr> test;
}

Here is my all_includes.hpp for binder:

#include <mlir/IR/BuiltinAttributeInterfaces.h>
#include <mlir/IR/BuiltinAttributes.h>

Here is my script that uses all the correct -I flags:

/home/mlevental/dev_projects/binder/build/bin/binder --root-module my_project --prefix \
 /home/mlevental/dev_projects/circt/hls/bindings \
  --bind "mlir" all_includes.hpp \
  -v \
   -- -std=c++17 \
    -I/home/mlevental/dev_projects/circt/cmake-build-debug-llvm/include \
    -I/home/mlevental/dev_projects/circt/cmake-build-debug-circt/include \
    -I/home/mlevental/dev_projects/circt/cmake-build-debug-llvm/tools/mlir/include \
    -I/home/mlevental/dev_projects/circt/cmake-build-debug-circt/tools/mlir/include \
    -I/home/mlevental/dev_projects/circt/llvm/llvm/include \
    -I/home/mlevental/dev_projects/circt/llvm/mlir/include \
    -I/home/mlevental/dev_projects/circt/include \
     -DMY_PROJECT_DEFINE -DNDEBUG

Note that the full path for

  1. BuiltinAttributeInterfaces.h is /home/mlevental/dev_projects/circt/llvm/mlir/include/mlir/IR/BuiltinAttributeInterfaces.h
  2. BuiltinAttributes.h is /home/mlevental/dev_projects/circt/llvm/mlir/include/mlir/IR/BuiltinAttributes.h
  3. BuiltinAttributes.h.inc, which is included in BuiltinAttributes.h (i.e. where AffineMapAttr is actually defined), is /home/mlevental/dev_projects/circt/cmake-build-debug-llvm/tools/mlir/include/mlir/IR/BuiltinAttributes.h.inc

I don't know what else to tell you and I also don't know how this is failing.

EDIT:

If I fullpath include BuiltinAttributeInterfaces.h.inc after BuiltinAttributeInterfaces.h then I get this contradictory set of errors:

In file included from /home/mlevental/dev_projects/circt/hls/bindings/all_includes.hpp:4:
/home/mlevental/dev_projects/circt/cmake-build-debug-llvm/tools/mlir/include/mlir/IR/BuiltinAttributeInterfaces.h.inc:410:7: error: redefinition of 'MemRefLayoutAttrInterface'
class MemRefLayoutAttrInterface : public ::mlir::AttributeInterface<MemRefLayoutAttrInterface, detail::MemRefLayoutAttrInterfaceInterfaceTraits> {

...

ERROR: Could not find binder for type: mlir::MemRefLayoutAttrInterface::Trait<mlir::AffineMapAttr>!
Usually cause for this is that type was only forward declared. Please check that Binder input include file have full declaration of this class.

@lyskov
Copy link
Member

lyskov commented Mar 11, 2022

Thank you for info @makslevental ! I think i got the idea of what might be going on here. Could you please try to explicitly disable bindings for mlir::AttributeInterface<MemRefLayoutAttrInterface, detail::MemRefLayoutAttrInterfaceInterfaceTraits> class and see if that helps? Another workaround would be disable bindings for class MemRefLayoutAttrInterface (unless of course you actually wanted it to be bound).

As i mentioned before: have you checked that full declaration of class mlir::MemRefLayoutAttrInterface::Trait is included in your all_includes.hpp?

@makslevental
Copy link
Author

makslevental commented Mar 11, 2022

mlir::MemRefLayoutAttrInterface::Trait is included in your all_includes.hpp?

I have answered this question in 10 different ways by now. I will answer again for the final time:

  1. There is no way that test.cpp could have compiled if the definition of that class wasn't somewhere in either mlir/IR/BuiltinAttributeInterfaces.h or mlir/IR/BuiltinAttributes.h or in a transitive #include of one of those.
  2. The class is defined in BuiltinAttributeInterfaces.h.inc (cf this gist) which is #included in BuiltinAttributeInterfaces.h which is in my all_includes.hpp.
  3. I cannot directly include that file because I will get redefinition errors.

Thank you for the project but at this point I'm moving on.

@lyskov
Copy link
Member

lyskov commented Mar 14, 2022

@makslevental i understand your frustration but please try to look at this from maintainer point of view: it is really impossible for one to read through all code of someone else projects which generate bugs (at least if you do not have infinite time which i do not!). The only realistic way this could work if bug reporter take time to isolate bug and provide minimal example that is easy reproduce and work with, - preferably single .hpp file with just a few class definitions. Please note that such minimal example can not be just bash script that links to LLVM sources tree (which by no means minimum!). I will be more then happy to debug (and fix if possible) this issue for you if you will take time to do that. Thanks,

Also, - thank you for linking LLVM files, - this makes debugging easier! Could you please post which LLVM version do you use for this?

re issue in general: it is possible for Binder to not be able to sort types so they could be bound without breaking dependencies. Example of this will be due to types circular dependencies. As i mention above simplest solution will be to disable bindings for one of such classes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants