This is an archive of the discontinued LLVM Phabricator instance.

Reland [llvm] Preliminary fat-lto-objects support
ClosedPublic

Authored by paulkirth on Mar 23 2023, 5:54 PM.

Details

Summary

Fat LTO objects contain both LTO compatible IR, as well as generated
object code. This allows users to defer the choice of whether to use LTO
or not to link-time. This is a feature available in GCC for some time,
and makes the existing -ffat-lto-objects flag functional in the same
way as GCC's.

Within LLVM, we add a new EmbedBitcodePass that serializes the module to
the object file, and expose a new pass pipeline for compiling fat
objects. The new pipeline initially clones the module and runs the
selected (Thin)LTOPrelink pipeline, after which it will serialize the
module into a .llvm.lto section of an ELF file. When compiling for
(Thin)LTO, this normally the point at which the compiler would emit a
object file containing the bitcode and metadata.

After that point we compile the original module using the
PerModuleDefaultPipeline used for non-LTO compilation. We generate
standard object files at the end of this pipeline, which contain machine
code and the new .llvm.lto section containing bitcode.

Since the two pipelines operate on different copies of the module, we
can be sure that the bitcode in the .llvm.lto section and object code
in .text are congruent with the existing output produced by the
default and LTO pipelines.

Original RFC: https://discourse.llvm.org/t/rfc-ffat-lto-objects-support/63977

Earlier versions of this patch were missing REQUIRES lines for llc
related tests in Transforms/EmbedBitcode. Those tests are now under
CodeGen/X86, which should avoid running the check on unsupported
platforms.

The EmbedbBitcodePass also returned PreservedAnalyses::all when adding a
metadata section, which failed expensive checks, since it modified the
module. This is now corrected.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
paulkirth marked 5 inline comments as done.May 25 2023, 2:17 PM
paulkirth added inline comments.
llvm/lib/Passes/PassBuilderPipelines.cpp
1485

I don't think the prior code mimic'ed the non-LTO pipeline, however. My understanding is that the prior version did the following for the non embedded code on which it would generate a native object:

(Thin)LTOPreLink + ModuleSimplification(ThinOrFullLTOPhase::None) + ModuleOptimization(ThinOrFullLTOPhase::None)

So you would have been essentially duplicating ModuleSimplification (since it is invoked in both the *LTO pre-link and again with LTO Phase = None). That can cause some issues because there are passes that are only meant to be invoked once that you could duplicate. A good example is PGO instrumentation/annotation (see the conditions under which addPGOInstrPasses is called from buildModuleSimplificationPipeline).

With your new version, my understanding is that you are calling:

  1. buildPerModuleDefaultPipeline on original Module -> generate native object
  2. (Thin)LTOPreLink on Module clone -> embed IR

And buildPerModuleDefaultPipeline is largely just ModuleSimplification + ModuleOptimization. So I'm not sure about why it would be more wasteful (other than cloning the Module) - you are not running more pipelines total than in the prior code. Unless I am misunderstanding the former or current code, which is possible!

I think that's an accurate summary. While my intent originally was to closely mimic the default pipeline, I don't think it was very close in reality. Also, as you pointed out the new approach isn't nearly as wasteful as I felt initially.

I agree ensuring full performance with FatLTO's non-LTO native object isn't terribly important. But my concern is whether this results in some unexpected behavior with certain passes not being run at all - e.g. ICP won't run at all if you are doing SamplePGO in this configuration.

...

As mentioned earlier, I think the question is whether it will be a surprise when using SamplePGO that certain things simply don't get run when using the non-LTO native objects. I don't know enough about when these will get used to know if this is a configuration anyone will care about. If it is, I suppose one approach would be to invoke the longer pipelines only under SamplePGO. Either way, this probably needs some clear documentation.

These are all performance concerns for the non-LTO native objects, not correctness right? I think if we clearly document that performance might be very meh for fat LTO non-LTO native objects this would be fine.

llvm/include/llvm/Bitcode/EmbedBitcodePass.h
33

is this default param necessary?

llvm/lib/Passes/PassBuilderPipelines.cpp
1265

do we still need IsFatLTO with the latest approach?

1485

IIUC the previous version was ThinLTOPreLink + ModuleOptimization(ThinOrFullLTOPhase::None), where we'd write out the module after pre-link and before module optimization to some global. So we'd avoid running the simplification pipeline twice with that. (the problem is that the simplification pipeline is slightly different between a normal build and a ThinLTO build)

the current version will run the simplification pipeline twice now, once for ThinLTO and once for the default pipeline

llvm/lib/Passes/PassRegistry.def
61

these should be parameterizable, see MODULE_PASS_WITH_PARAMS

paulkirth marked an inline comment as done.May 25 2023, 2:37 PM

These are all performance concerns for the non-LTO native objects, not correctness right? I think if we clearly document that performance might be very meh for fat LTO non-LTO native objects this would be fine.

In the prior version, I think its too hard to say if running pipelines, like ModuleSimplification, multiple times is safe/correct without some fairly detailed analysis. I can easily imagine any transform that is only expected to run once may introduce a subtle bug if run again on the same module. Nothing obvious here jumps out at me, but I'm also not confident enough to say that it can't happen.

That said, since FatLTO no longer use a modified/custom pipeline but uses the (Thin)LTO and Module piplines on independed modules, there should no longer be any correctness nor performance issues over a normal compilation for the non-LTO object code.

llvm/include/llvm/Bitcode/EmbedBitcodePass.h
33

IIRC it was required for some reason, but let me double check that.

llvm/lib/Passes/PassBuilderPipelines.cpp
1265

That's a good point. I don't think so, so I can simplify this futher. Thanks for pointing this out.

llvm/lib/Passes/PassRegistry.def
61

That is a much better way to handle this. Thank you.

paulkirth updated this revision to Diff 525901.May 25 2023, 6:29 PM
paulkirth edited the summary of this revision. (Show Details)

Update summary, documentation, and address comments.

aeubanks added inline comments.May 30 2023, 10:59 AM
llvm/include/llvm/Bitcode/EmbedBitcodePass.h
27

extraneous ;

29

this is never used

44

if you really wanted to hook this up to the pipeline parsing (so you could specify a sub-passmanager) you'd have to thread through all the parsing code. probably not worth it

llvm/include/llvm/Passes/PassBuilder.h
242

comment out of date

llvm/lib/Passes/PassBuilderPipelines.cpp
1265

still not removed?

1471

this comment is obsolete now

paulkirth updated this revision to Diff 527074.May 31 2023, 9:03 AM
paulkirth marked 7 inline comments as done.

Address remaining comments

  • Fix Doc string
  • Remove obsolete comment
  • Remove uses of IsThinkLTO
  • Remove some dead code
  • Fix some typos
  • Fix test looking for wrong pass name in the error string
  • git clang-format
llvm/lib/Passes/PassBuilderPipelines.cpp
1471

I was thinking once that patch lands we can take the ThinOrFullLTOPhase direcly and avoid the branch, but maybe it still makes sense to leave this as is.

this basically lgtm

llvm/lib/Bitcode/Writer/EmbedBitcodePass.cpp
31–38

these should be unnecessary with the newly added pass params, e.g. -passes=embed-bitcode<emit-summary>

paulkirth updated this revision to Diff 529076.Jun 6 2023, 4:07 PM

Rebase

  • Fix typo in docs
  • Remove unneeded cl::opts
paulkirth marked an inline comment as done.Jun 6 2023, 4:08 PM

@tejohnson @nikic any lingering concerns?

tejohnson accepted this revision.Jun 9 2023, 4:34 PM

LGTM with a couple of minor nits and a question back to @nikic about one of this comments.

llvm/lib/Bitcode/Writer/EmbedBitcodePass.cpp
31

true seems to be the default, did you mean false? The same could probably be said for the later call to report_fatal_error (ELF format). I actually think this one seems less like a user error and more like a compiler error (if this pass gets run twice somehow).

49

Document constant parameter (nullptr)

llvm/test/Bitcode/embed.ll
15

s/sue/sure

Thanks for the feedback @tejohnson. I'm about to take off for the weekend, but I'll address these on Monday.

llvm/lib/Bitcode/Writer/EmbedBitcodePass.cpp
31

Oh, yeah, probably. I do see your point though. I guess maybe user error encompasses adding a pass multiple times?

IMO they're all kind of hard errors. I'm happy to do this either way, but if we want to suppress this one, then you're right that it should be false here.

49

ah, good catch. Thanks.

llvm/test/Bitcode/embed.ll
15

ty

nikic added a comment.Jun 10 2023, 1:09 AM

I'm okay with the new approach -- I think it makes the feature somewhat less compelling, but it's the path of least resistance for now. However, I'd like to have it documented that we're explicitly not guaranteeing that the results are bit-for-bit identical, and this is just an artifact of the current implementation.

Two notes on the larger feature:

  • I'm concerned about the lld-only limitation. Unless there is some technical limitation here, this should be supported in LLVMgold as well.
  • It looks like this requires passing -fat-lto-objects to lld to actually get LTO -- and it looks like the clang doesn't do this itself even if -ffat-lto-objects is passed to the driver? Can you please check how these flags work in GCC? I'm not entirely confident on this, but I expect that if you have fat objects they will use LTO by default unless you pass -fno-lto to the driver. We should try to match whatever the GCC behavior is here.
llvm/docs/FatLTOSupport.rst
28
29
34
38–40
43

The pre-link pipeline is run as part of EmbedBitcode, not before it.

50

This seems like the important part for end users, so maybe put that in the introduction.

60

This is a very annoying limitation -- is it particularly hard to implementation support for this in the LLVMgold plugin?

We won't be able to use fat objects if it's only supported by LLD.

llvm/docs/ReleaseNotes.rst
85

Maybe link the docs instead / as well here?

llvm/lib/Bitcode/Writer/EmbedBitcodePass.cpp
31

Sorry, I meant false here.

My general rule of thumb is that if you can write a test for it, then it should not crash.

I'm okay with the new approach -- I think it makes the feature somewhat less compelling, but it's the path of least resistance for now. However, I'd like to have it documented that we're explicitly not guaranteeing that the results are bit-for-bit identical, and this is just an artifact of the current implementation.

Two notes on the larger feature:

  • I'm concerned about the lld-only limitation. Unless there is some technical limitation here, this should be supported in LLVMgold as well.

Great point. I see that the linker changes aren't in this patch, but in another related one. I agree it should be supported by the LLVM gold plugin as well, which should be straightforward. This can be another follow on patch. It looks like you would want to modify this change made to ignore the -fembed-bitcode sections so that we do in fact claim the fat object files:
https://github.com/llvm/llvm-project/blob/main/llvm/tools/gold/gold-plugin.cpp#L541-L547

  • It looks like this requires passing -fat-lto-objects to lld to actually get LTO -- and it looks like the clang doesn't do this itself even if -ffat-lto-objects is passed to the driver? Can you please check how these flags work in GCC? I'm not entirely confident on this, but I expect that if you have fat objects they will use LTO by default unless you pass -fno-lto to the driver. We should try to match whatever the GCC behavior is here.

agree

paulkirth updated this revision to Diff 531532.Jun 14 2023, 3:15 PM
paulkirth marked 8 inline comments as done.

Rebase and address comments

paulkirth added inline comments.Jun 15 2023, 3:25 PM
llvm/test/Bitcode/embed-multiple.ll
2

looks like I failed to update the tests after making the errors non-fatal.

Rebase and fix test cases.

nikic accepted this revision.Jun 16 2023, 12:01 PM

LGTM

llvm/docs/FatLTOSupport.rst
38

unmodified

61

I feel like this information has been repeated (with minor variations) three times: In the bullet points above, the paragraph following it, and then here...

74

Despite the name, the gold plugin is also supported by the bfd linker, so you actually support all common linkers (on ELF based systems).

This revision is now accepted and ready to land.Jun 16 2023, 12:01 PM

Apologies for not commenting soon. I'll take a close look shortly.

Apologies for not commenting soon. I'll take a close look shortly.

TY!

llvm/docs/FatLTOSupport.rst
61

Yeah, it's definitely a bit redundant. I'll go through this one more time and try to streamline the text.

74

Ah, right. I completely forgot bfd could use the plugin.

paulkirth updated this revision to Diff 532270.Jun 16 2023, 1:01 PM

Revise text in documentation.

paulkirth updated this revision to Diff 532271.Jun 16 2023, 1:04 PM
paulkirth marked 3 inline comments as done.

Document constant parameter.

paulkirth added inline comments.Jun 16 2023, 1:05 PM
llvm/docs/FatLTOSupport.rst
61

I feel like the current version reads a bit better. WDYT?

nikic added inline comments.Jun 17 2023, 12:55 AM
llvm/docs/FatLTOSupport.rst
50
61

Looks good!

62
MaskRay requested changes to this revision.EditedJun 17 2023, 3:57 PM

Started looking...

-DBUILD_SHARED_LIBS=on uses -Wl,-z,defs and can detect some library layering (https://llvm.org/docs/CodingStandards.html#library-layering https://maskray.me/blog/2021-06-13-dependency-related-linker-options) problems. ninja LLVMBitWriter doesn't build due to missing dependencies.

One issue can be addressed with the following change but the dependency may be a bit weird.

diff --git i/llvm/lib/Bitcode/Writer/CMakeLists.txt w/llvm/lib/Bitcode/Writer/CMakeLists.txt
index 2b17aa912016..51849d2d0fc0 100644
--- i/llvm/lib/Bitcode/Writer/CMakeLists.txt
+++ w/llvm/lib/Bitcode/Writer/CMakeLists.txt
@@ -17,2 +17,4 @@ add_llvm_component_library(LLVMBitWriter
   TargetParser
+  TransformUtils
   )

The other issue (error: undefined symbol: llvm::ThinLTOBitcodeWriterPass::run) is more tricky: LLVMipo (llvm/lib/Transforms/IPO/CMakeLists.txt) depends on LLVMBitWriter, so LLVMBitWriter cannot depend on LLVMipo.

llvm/lib/Bitcode/Writer/EmbedBitcodePass.cpp
25

Including llvm/Transforms headers seems problematic.

This revision now requires changes to proceed.Jun 17 2023, 3:57 PM
MaskRay added inline comments.Jun 17 2023, 4:36 PM
llvm/docs/FatLTOSupport.rst
2

Maybe just FatLTO.rst. For other features, we don't use the Support suffix.

19
llvm/lib/Bitcode/Writer/EmbedBitcodePass.cpp
33

Nit: diagnostics do not need capitalization (https://llvm.org/docs/CodingStandards.html#error-and-warning-messages) but report_fatal_error messages often do capitalization, so I think capitalization is fine, but the period is often omitted.

56

You can delete ModuleData and even Buf.

llvm/lib/Passes/PassRegistry.def
175
llvm/test/Bitcode/embed.ll
10

Nit: complete sentences in comments end with a period.

I think we need a new section type (say, SHT_LLVM_LTO) in llvm/include/llvm/BinaryFormat/ELF.h. Then add a test llvm/test/MC/ELF to use llvm-readobj -S to test this new section type.

.llvm.lto should be added beside .llvmcmd in llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp so that .llvm.lto has the SHF_EXCLUDE flag on ELF.
We need a llvm/test/CodeGen/ test using llvm-readelf -S to test the section flags (E).

paulkirth marked 4 inline comments as done.Jun 20 2023, 10:01 AM

Started looking...

-DBUILD_SHARED_LIBS=on uses -Wl,-z,defs and can detect some library layering (https://llvm.org/docs/CodingStandards.html#library-layering https://maskray.me/blog/2021-06-13-dependency-related-linker-options) problems. ninja LLVMBitWriter doesn't build due to missing dependencies.

One issue can be addressed with the following change but the dependency may be a bit weird.

diff --git i/llvm/lib/Bitcode/Writer/CMakeLists.txt w/llvm/lib/Bitcode/Writer/CMakeLists.txt
index 2b17aa912016..51849d2d0fc0 100644
--- i/llvm/lib/Bitcode/Writer/CMakeLists.txt
+++ w/llvm/lib/Bitcode/Writer/CMakeLists.txt
@@ -17,2 +17,4 @@ add_llvm_component_library(LLVMBitWriter
   TargetParser
+  TransformUtils
   )

The other issue (error: undefined symbol: llvm::ThinLTOBitcodeWriterPass::run) is more tricky: LLVMipo (llvm/lib/Transforms/IPO/CMakeLists.txt) depends on LLVMBitWriter, so LLVMBitWriter cannot depend on LLVMipo.

What if we move the pass into transforms? Yes, its fairly correlated to bitcode writing, but the pass is really just creating a new section using existing utilities, which could just be considered a transform. There's also an argument that this pass should just live wherever the ThinLTOBitcodeWritterPass does. I think that should fix both the issues you've mentioned here.

Do you think that would be a better solution?

Address comments about documentation.

I still need to address more of @MaskRay's comments.

Started looking...

-DBUILD_SHARED_LIBS=on uses -Wl,-z,defs and can detect some library layering (https://llvm.org/docs/CodingStandards.html#library-layering https://maskray.me/blog/2021-06-13-dependency-related-linker-options) problems. ninja LLVMBitWriter doesn't build due to missing dependencies.

One issue can be addressed with the following change but the dependency may be a bit weird.

diff --git i/llvm/lib/Bitcode/Writer/CMakeLists.txt w/llvm/lib/Bitcode/Writer/CMakeLists.txt
index 2b17aa912016..51849d2d0fc0 100644
--- i/llvm/lib/Bitcode/Writer/CMakeLists.txt
+++ w/llvm/lib/Bitcode/Writer/CMakeLists.txt
@@ -17,2 +17,4 @@ add_llvm_component_library(LLVMBitWriter
   TargetParser
+  TransformUtils
   )

The other issue (error: undefined symbol: llvm::ThinLTOBitcodeWriterPass::run) is more tricky: LLVMipo (llvm/lib/Transforms/IPO/CMakeLists.txt) depends on LLVMBitWriter, so LLVMBitWriter cannot depend on LLVMipo.

What if we move the pass into transforms? Yes, its fairly correlated to bitcode writing, but the pass is really just creating a new section using existing utilities, which could just be considered a transform. There's also an argument that this pass should just live wherever the ThinLTOBitcodeWritterPass does. I think that should fix both the issues you've mentioned here.

Do you think that would be a better solution?

yes living alongside ThinLTOBitcodeWritterPass makes sense

paulkirth marked 12 inline comments as done.

Address some comments

  • Move EmbedBitcodePass into Transforms/IPO
  • Fix misc typos + formatting
  • Simplify some code in EmbedBitcodePass

I have not addressed anything related to the SHT_LLVM_LTO flag or handling of .llvm.lto yet.

paulkirth added inline comments.Jun 20 2023, 12:10 PM
llvm/lib/Transforms/IPO/EmbedBitcodePass.cpp
54–57 ↗(On Diff #532989)

Looks like I forgot to remove these lines.

paulkirth added inline comments.Jun 20 2023, 12:51 PM
llvm/lib/Transforms/IPO/EmbedBitcodePass.cpp
58 ↗(On Diff #532989)

Also, while I'm not arguing that we shouldn't add SHF_LLVM_LTO, embedBufferInModule does mark it as MD_exclude https://github.com/llvm/llvm-project/blob/f1a040298381cdfc0657d5ecba231ebe6bbef61a/llvm/lib/Transforms/Utils/ModuleUtils.cpp#L375, which will eventually become SHF_EXCLUDE.

MaskRay added a comment.EditedJun 20 2023, 1:54 PM

Thanks for the update. This fixes -DBUILD_SHARED_LIBS=on build of mine.

Perhaps llvm/test/Bitcode/ tests (which don't normally test -passes=) should be moved to elsewhere, e.g. llvm/test/Transforms/EmbedBitcodePass/ ?

Another strange thing to fix:

llc a.ll => .type .Lllvm.embedded.object,@object

We don't normally set attributes on temporary symbols (.L), which do not normally have symbol table entries anyway.

llvm/lib/Transforms/IPO/EmbedBitcodePass.cpp
9 ↗(On Diff #532989)

If EmbedBitcodePass.h provides a descriptive comment, this comment can probably be removed. It does not give additional information to users:)

42 ↗(On Diff #532989)

delete unneeded blank line

47 ↗(On Diff #532989)

delete unneeded blank line

MaskRay accepted this revision.Jun 20 2023, 2:03 PM

Thanks for the update. This fixes -DBUILD_SHARED_LIBS=on build of mine.

Perhaps llvm/test/Bitcode/ tests (which don't normally test -passes=) should be moved to elsewhere, e.g. llvm/test/Transforms/EmbedBitcodePass/ ?

Another strange thing to fix:

llc a.ll => .type .Lllvm.embedded.object,@object

We don't normally set attributes on temporary symbols (.L), which do not normally have symbol table entries anyway.

This is due to private but I think this is acceptable. Both GNU assembler and LLVM integrated assembler do not create a symbol table entry, so this is fine. This likely does not justify adding a special case for private lowering to the underlying ELF section.

Feel free to land this one with a readelf -S test to check SHF_EXCLUDE. Maybe give some time for the Clang patch. Landing this one shall make reviewing the Clang part easier.

llvm/docs/FatLTO.rst
62 ↗(On Diff #532989)

LLVMgold.so plugin.

Consider using hypertext to link this to llvm/docs/GoldPlugin.rst. I don't recall immediately the reST syntax...

This revision is now accepted and ready to land.Jun 20 2023, 2:03 PM

Thanks for the update. This fixes -DBUILD_SHARED_LIBS=on build of mine.

Perhaps llvm/test/Bitcode/ tests (which don't normally test -passes=) should be moved to elsewhere, e.g. llvm/test/Transforms/EmbedBitcodePass/ ?

Oh, yes. I should have moved those too. I'll take care of that in the next version

Thanks for the update. This fixes -DBUILD_SHARED_LIBS=on build of mine.

Perhaps llvm/test/Bitcode/ tests (which don't normally test -passes=) should be moved to elsewhere, e.g. llvm/test/Transforms/EmbedBitcodePass/ ?

Another strange thing to fix:

llc a.ll => .type .Lllvm.embedded.object,@object

We don't normally set attributes on temporary symbols (.L), which do not normally have symbol table entries anyway.

This is due to private but I think this is acceptable. Both GNU assembler and LLVM integrated assembler do not create a symbol table entry, so this is fine. This likely does not justify adding a special case for private lowering to the underlying ELF section.

I'm not sure I follow the conversation here. Is this an issue w/ the defaults used by embedBufferInModule()? or is the oddity just related to the symol being private?

Feel free to land this one with a readelf -S test to check SHF_EXCLUDE. Maybe give some time for the Clang patch. Landing this one shall make reviewing the Clang part easier.

Will do. Thanks for all the feedback.

llvm/lib/Transforms/IPO/EmbedBitcodePass.cpp
9 ↗(On Diff #532989)

noted. I'll expand the description in the header and remove this.

paulkirth marked 4 inline comments as done.

Address comments

  • Add test for SHT_EXCLUDE
  • Fix some formatting
  • Improve documentation/description in header
  • Update documentation links
This revision was landed with ongoing or failed builds.Jun 23 2023, 10:51 AM
This revision was automatically updated to reflect the committed changes.
paulkirth reopened this revision.Jun 23 2023, 11:49 AM

Reopening until I can investigate the buildbot failures.

This revision is now accepted and ready to land.Jun 23 2023, 11:49 AM
MaskRay added inline comments.Jun 23 2023, 12:53 PM
llvm/test/Transforms/EmbedBitcode/embed.ll
23 ↗(On Diff #534016)

We normally indent continuation lines with 2 spaces

43 ↗(On Diff #534016)

delete trailing blank line

MaskRay requested changes to this revision.Jun 23 2023, 12:54 PM
MaskRay added inline comments.
llvm/test/Transforms/EmbedBitcode/embed.ll
1 ↗(On Diff #534016)

llc needs REQUIRES: x86-registered-target

This revision now requires changes to proceed.Jun 23 2023, 12:54 PM
MaskRay added inline comments.Jun 23 2023, 12:56 PM
llvm/test/Transforms/EmbedBitcode/embed.ll
24 ↗(On Diff #534016)

--elf-output-style=JSON --pretty-print is not normal. llvm-readelf -S output is much conciser.

paulkirth updated this revision to Diff 534065.Jun 23 2023, 1:53 PM
paulkirth marked 4 inline comments as done.

Fix test code

  • move llc check for SHF_EXCLUDE under CodeGen/X86
  • fix formatting
paulkirth updated this revision to Diff 534070.Jun 23 2023, 2:03 PM
paulkirth retitled this revision from [llvm] Preliminary fat-lto-objects support to Reland [llvm] Preliminary fat-lto-objects support.
paulkirth edited the summary of this revision. (Show Details)

Update summary and title for reland

paulkirth updated this revision to Diff 534074.Jun 23 2023, 2:17 PM

Remove some unused headers & fix formatting of the include guards

MaskRay accepted this revision.Jun 23 2023, 3:55 PM

Thanks! One nit in the test.

llvm/test/CodeGen/X86/fat-lto-section.ll
8 ↗(On Diff #534074)

I typically just drop [Nr] and [ 4] from the output so that adding a new section will not cause the section index to shuffle and cause the test to fail.

This revision is now accepted and ready to land.Jun 23 2023, 3:55 PM
paulkirth updated this revision to Diff 534114.Jun 23 2023, 4:22 PM
paulkirth marked an inline comment as done.

Make test less brittle to ordering changes

This revision was landed with ongoing or failed builds.Jun 23 2023, 4:24 PM
This revision was automatically updated to reflect the committed changes.
paulkirth reopened this revision.Jun 28 2023, 11:10 AM

This seems to have failed on expensive checks because we marked the pass as PreservedAnalyses::all(). I guess the simplest thing to do is to change that to none, but I think there should be something else we can do here to avoid invalidating analyses that aren't affected by inserting the metadata section.

This revision is now accepted and ready to land.Jun 28 2023, 11:10 AM
paulkirth added inline comments.Jun 28 2023, 11:13 AM
llvm/lib/Transforms/IPO/EmbedBitcodePass.cpp
51 ↗(On Diff #534116)

Is there another option we can use here. none seems like overkill. Since this is basically the first thing to run on the module, I'm not sure if there is much point in trying to do anything smarter, but it certainly doesn't feel right.

aeubanks added inline comments.Jun 28 2023, 11:15 AM
llvm/lib/Transforms/IPO/EmbedBitcodePass.cpp
51 ↗(On Diff #534116)

perhaps something like https://reviews.llvm.org/D153855?

paulkirth edited the summary of this revision. (Show Details)

Don't preserve analysis to satisfy expensive checks

paulkirth updated this revision to Diff 535511.Jun 28 2023, 1:49 PM

Modify the structural hash since we don't modify the module except to add a
metadata section.

paulkirth marked an inline comment as done.Jun 28 2023, 1:50 PM
paulkirth added inline comments.
llvm/lib/Transforms/IPO/EmbedBitcodePass.cpp
51 ↗(On Diff #534116)

That seems like it will work. Thanks for the suggestion. We can go back to none if we're unhappy about amending the structural hash.

aeubanks added inline comments.Jun 28 2023, 1:51 PM
llvm/lib/IR/StructuralHash.cpp
62 ↗(On Diff #535511)

maybe we should exclude anything that begins with llvm., but that should be a separate patch, so this lgtm

paulkirth updated this revision to Diff 535513.Jun 28 2023, 1:52 PM
paulkirth marked an inline comment as done.

Update comment.

paulkirth added inline comments.Jun 28 2023, 2:30 PM
llvm/lib/IR/StructuralHash.cpp
62 ↗(On Diff #535511)

Filed https://github.com/llvm/llvm-project/issues/63590. I can make the follow up patch, but I may not get a chance till tomorrow.

This revision was landed with ongoing or failed builds.Jun 28 2023, 2:38 PM
This revision was automatically updated to reflect the committed changes.