[PATCH] D140420: [Support] Update modulemap for TargetParser

[Steven Wu via Phabricator via llvm-commits]

steven_wu added inline comments.


================
Comment at: llvm/include/llvm/module.modulemap:403
+    // Exclude this; it is a forwarding header to TargetParser.
+    exclude header "ADT/Triple.h"
   }
----------------
lenary wrote:
> steven_wu wrote:
> > lenary wrote:
> > > steven_wu wrote:
> > > > lenary wrote:
> > > > > steven_wu wrote:
> > > > > > I am not sure if this exclude is necessary. It is ok for ADT to depend on TargetParser, correct?
> > > > > > 
> > > > > > We should either clean up all the include through ADT, or maybe just leave this. Same for all the TargetParser header in Support.
> > > > > I got build issues until I added this. The `llvm/ADT/Triple.h` file now just forwards to `llvm/TargetParser/Triple.h`, so without this, you get a circular dependency of LLVM_Support -> TargetParser -> LLVM_Support
> > > > But isn't Triple.h module dependency: ADT -> TargetParser? I don't see an edge goes back.
> > > > 
> > > > For the rest of the TargetParser headers, that could be a problem. What prevents us from just rewrite all the includes to use the new location and remove those forwarding headers? Just excluding here without including them in a different module still means they cannot be used in a module build.
> > > As I understand it, without the exclude line, if something outside ADT or Targetparser includes on "llvm/ADT/*" then it's classed as pulling in the ADT module. In the case of "llvm/ADT/Triple.h", this pulls in TargetParser (because it is a forwarding include to "llvm/TargetParser/Triple.h", which is classed as being in TargetParser, because it is in "llvm/TargetParser/*"). Then when TargetParser relies on something else in ADT, you get a circular dependency. 
> > > 
> > > This exclude line says "don't consider this header part of ADT" (because it isn't, any more). I guess I'm missing that I need to add the forwarding headers to the new TargetParser module, even though they're in the "wrong" directory. 
> > > 
> > > The forwarding headers are staying for at least LLVM 16, to make compatibility easier, especially for downstream users. I intend to rewrite the in-tree uses of the forwarding headers soon, but I'm sick right now and won't be back online until after the holiday period. 
> > I won't be worry too much about compatibility considering how many things are being rewritten now that almost nothing will be compatible~
> > 
> > As far as I care, as long as the module bots stay in good shape I am good. @aprantl what do you think?
> Forwarding headers were specifically requested in the discourse RFC. 
> 
> I think I finally got my build working with `LLVM_ENABLE_MODULES=On` (I also had to swap to using libc++, and turn off `BUILD_SHARED_LIBS=On`), so I think the newest version of the patch should work, but I would welcome someone testing it in a modules build.
> 
> I haven't yet found a public modules buildbot, either, maybe I just don't know where to look.
The most jobs on green dragon Jenkins builds with modules, for example: https://green.lab.llvm.org/green/job/clang-stage1-RA/

Not all backends are turned on though so it might not catch all the module failures but it is a good start. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140420/new/

https://reviews.llvm.org/D140420