[Support] Update modulemap for TargetParser

[lenary]

Summary

This is a follow-up to 6bdf378dcd349d97152846bb687c1d1de511d138, to show
that we did actually manage to split out TargetParser, but forwarding
headers still remain.

[lenary]

I hope this more clearly shows that the TargetParser is no longer entwined with Support any more.

[lenary]

This might need to wait until we've moved over to the non-forwarding headers entirely. I know @steven_wu started on this, I'm not sure how quickly I should start doing the rest of that work.

[steven_wu]

llvm/include/llvm/module.modulemap

403

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.

[lenary]

llvm/include/llvm/module.modulemap

403

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

[steven_wu]

llvm/include/llvm/module.modulemap

403

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.

[lenary]

llvm/include/llvm/module.modulemap

403

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.

[steven_wu]

llvm/include/llvm/module.modulemap

403

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?

[lenary]

llvm/include/llvm/module.modulemap

403

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.

[steven_wu]

llvm/include/llvm/module.modulemap

403

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.

[lenary]

Most recent updates were to address changes in rGc570287fbf4d

I believe this should be landable, and is a clearer "fix" for the modulemap, so that the targetparser is a separate module. I've tried it on linux, using libc++ with clang modules turned on, and the build succeeded, where it wasn't when the main TargetParser patch was freshly landed.

[lenary]

I noticed some duplicate lines in my previous update, corrected now.

Adding @dblaikie as a reviewer as he also reported problems with the module build.

[dblaikie]

In D140420#4025376, @lenary wrote:

I noticed some duplicate lines in my previous update, corrected now.

Adding @dblaikie as a reviewer as he also reported problems with the module build.

Thanks! The modulemaps aren't an issue for us (Google) - we use the Bazel build files and explicit modules builds that don't use modulemaps.

I'm looking at the BUILD files and I'm not entirely sure how they work in the face of this issue... but they seem to be working/seems we don't have an issue anymore...

[lenary]

In D140420#4027204, @dblaikie wrote:

In D140420#4025376, @lenary wrote:

I noticed some duplicate lines in my previous update, corrected now.

Adding @dblaikie as a reviewer as he also reported problems with the module build.

Thanks! The modulemaps aren't an issue for us (Google) - we use the Bazel build files and explicit modules builds that don't use modulemaps.

I'm looking at the BUILD files and I'm not entirely sure how they work in the face of this issue... but they seem to be working/seems we don't have an issue anymore...

Sorry for the confusion about modules builds and modulemaps. I love that we have the same words for several different things, and so many separate build configurations.

I think MaskRay fixed the Bazel build files, by making TargetParser just another part of the Support component (in rGbb22e3e8c006debb37efc04fbacb863290ec3484, though there is also now a :TargetParser component since rGc2a5f156d2977ff471934ebe6f143bc569fc4bc9 , so the CPP files are part of two components!). It would be good to do the equivalent of this patch there too, but I don't know how bazel modules cope with forwarding headers. I'm not untangling the bazel configuration, I am not an expert in it.

[dblaikie]

In D140420#4027284, @lenary wrote:

In D140420#4027204, @dblaikie wrote:

In D140420#4025376, @lenary wrote:

I noticed some duplicate lines in my previous update, corrected now.

Adding @dblaikie as a reviewer as he also reported problems with the module build.

Thanks! The modulemaps aren't an issue for us (Google) - we use the Bazel build files and explicit modules builds that don't use modulemaps.

I'm looking at the BUILD files and I'm not entirely sure how they work in the face of this issue... but they seem to be working/seems we don't have an issue anymore...

Sorry for the confusion about modules builds and modulemaps. I love that we have the same words for several different things, and so many separate build configurations.

I think MaskRay fixed the Bazel build files, by making TargetParser just another part of the Support component (in rGbb22e3e8c006debb37efc04fbacb863290ec3484, though there is also now a :TargetParser component since rGc2a5f156d2977ff471934ebe6f143bc569fc4bc9 , so the CPP files are part of two components!). It would be good to do the equivalent of this patch there too, but I don't know how bazel modules cope with forwarding headers. I'm not untangling the bazel configuration, I am not an expert in it.

yeah, sorry, I just meant "seems we've got something working for the Bazel build files, which was mostly my concern, so I don't have opinions on the modulemaps".

I think the layering is important in general, but understand that forwarding header for backcompat during migration is useful - & probably the simplest fix is to classify the forwarding header as part of the TargetParser library, despite being in the Support directory, which I guess is what you're doing here - so seems like a reasonable compromise.

[lenary]

@steven_wu @aprantl ping

[lenary]

@steven_wu @aprantl @dblaikie I'm abandoning this change, because as of rGd768bf994f50 these headers are not used in LLVM (apart from in Polly's isl import, which I have contacted the isl maintainer about). Hopefully this means you can update your clang modules/bazel configurations to properly consider the TargetParser separately from Support, if you wish.

llvm/include/llvm/module.modulemap

 // A module covering ADT/ and Support/. These are intertwined and
 // codependent, and notionally form a single module.
 module LLVM_Utils {
   module ADT {
     requires cplusplus
 
     umbrella "ADT"
     module * { export * }
+
+    // Exclude this; it is a forwarding header to TargetParser.
+    exclude header "ADT/Triple.h"
   }
 
   module Support {
     requires cplusplus
 
     umbrella "Support"
     module * { export * }
 
     // Exclude this; it should only be used on Windows.
     exclude header "Support/Windows/WindowsSupport.h"
 
     // Exclude these; they are fundamentally non-modular.
     exclude header "Support/PluginLoader.h"
     exclude header "Support/Solaris/sys/regset.h"
+
     textual header "Support/TargetOpcodes.def"
 
-  }
+    // Exclude these; they are forwarding headers to TargetParser.
+    exclude header "Support/TargetParser.h"
+    exclude header "Support/AArch64TargetParser.h"
+    exclude header "Support/AArch64TargetParser.h"
+    exclude header "Support/ARMTargetParser.h"
+    exclude header "Support/ARMTargetParserCommon.h"
+    exclude header "Support/CSKYTargetParser.h"
+    exclude header "Support/Host.h"
+    exclude header "Support/LoongArchTargetParser.h"
+    exclude header "Support/RISCVISAInfo.h"
+    exclude header "Support/TargetParser.h"
+    exclude header "Support/X86TargetParser.def"
+    exclude header "Support/X86TargetParser.h"
+  }
+
+  // This part of the module is usable from both C and C++ code.
+  module ConvertUTF {
+    header "Support/ConvertUTF.h"
+    export *
+  }
 
-  module TargetParser {
-    requires cplusplus
+module LLVM_TargetParser {
 
-    umbrella "TargetParser"
-    module * { export * }
 
-    // These are intended for textual inclusion.
-    textual header "TargetParser/AArch64TargetParser.def"
-    textual header "TargetParser/ARMTargetParser.def"
-    textual header "TargetParser/CSKYTargetParser.def"
-    textual header "TargetParser/RISCVTargetParser.def"
-    textual header "TargetParser/X86TargetParser.def"
-    textual header "TargetParser/LoongArchTargetParser.def"
-  }
 
-  // This part of the module is usable from both C and C++ code.
-  module ConvertUTF {
-    header "Support/ConvertUTF.h"
-    export *
-  }
-}
-
 // This is used for a $src == $build compilation. Otherwise we use
 // LLVM_Support_DataTypes_Build, defined in a module map that is
 // copied into the build area.
 module LLVM_Support_DataTypes_Src {
   header "llvm/Support/DataTypes.h"
   export *
 }
 
 module LLVM_WindowsManifest {
   requires cplusplus
   umbrella "WindowsManifest"
   module * { export * }
 }