Page MenuHomePhabricator

[IR] make -stack-alignment= into a module attr
ClosedPublic

Authored by nickdesaulniers on May 24 2021, 2:28 PM.

Details

Summary

Similar to D102742, specifying the stack alignment via CodegenOpts means
that this flag gets dropped during LTO, unless the command line is
re-specified as a plugin opt. Instead, encode this information as a
module level attribute so that we don't have to expose this llvm
internal flag when linking the Linux kernel with LTO.

Looks like external dependencies might need a fix:

Link: https://github.com/ClangBuiltLinux/linux/issues/1377

Diff Detail

Event Timeline

nickdesaulniers requested review of this revision.May 24 2021, 2:28 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMay 24 2021, 2:28 PM
RKSimon resigned from this revision.May 27 2021, 2:47 AM
RKSimon added a reviewer: jrtc27.

bumping for review (or suggestions of additional reviewers)

Curiously, using ModFlagBehavior::Error doesn't error if one of two modules being linked together doesn't have such a module level attribute.

Yeah, there's a Require behavior, but that only allows you to specify what the value should be after linking modules. Barring changing the behavior of Error, which I'm guessing is relied upon too many places, the main thing I can think of is to add a new module flag behavior with tbd name that is essentially like Error but treats a module without the module flag as having a conflicting value and issues an error for that as well.

llvm/test/Linker/stack-alignment.ll
12

Will you get this error currently? I thought per comment and behavior of Error that it shouldn't give an error.

Curiously, using ModFlagBehavior::Error doesn't error if one of two modules being linked together doesn't have such a module level attribute.

Yeah, there's a Require behavior, but that only allows you to specify what the value should be after linking modules. Barring changing the behavior of Error, which I'm guessing is relied upon too many places, the main thing I can think of is to add a new module flag behavior with tbd name that is essentially like Error but treats a module without the module flag as having a conflicting value and issues an error for that as well.

I agree, that's what I was thinking we'd need to do, so I'm glad you came to a similar conclusion. I'll work on implementing a new ModFlagBehavior in this patch implementing such semantics that then this module level IR node can use.

nickdesaulniers added inline comments.Jun 7 2021, 3:48 PM
llvm/test/Linker/stack-alignment.ll
12

the RUN line for that check is missing the :, so it's not actually run. I think I'll create the new ModFlagBehavior as a child revision to this commit, in which I add the new behavior and upgrade this attribute to use it, adding in this unit test.


Now that I've had time to play with implementing such a ModFlagBehavior; it looks like it's not possible to create a module with such semantics then test it properly with llvm-link. The reason is that llvm-link is structured to start with an empty module ("Composite"), then link in the first module specified on the command line, then link in the rest of the modules specified. So we can't disambiguate between the initial empty module being linked against the initial source file with the MDNode vs 2 full modules where 1 is missing the MD node. (ie. for two input source files, the main module linking logic is run twice, not once).

See: https://reviews.llvm.org/D103851


In that case, I just plan to remove this test case then, since it wasn't being RUN anyways.

  • remove test for module without module attr
nickdesaulniers marked an inline comment as done.Jun 7 2021, 4:06 PM
tejohnson accepted this revision.Jun 7 2021, 11:16 PM

lgtm

llvm/test/Linker/stack-alignment.ll
12

Ah ok, that makes sense. For regular LTO via the linker we also start with an empty module and link into it, so it would have the same issue.

Presumably we could detect and handle the case of linking into a completely empty module? But that can be revisited later.

This revision is now accepted and ready to land.Jun 7 2021, 11:16 PM
nickdesaulniers edited the summary of this revision. (Show Details)Jun 8 2021, 8:16 AM
nickdesaulniers edited the summary of this revision. (Show Details)Jun 8 2021, 8:20 AM
This revision was landed with ongoing or failed builds.Jun 8 2021, 8:31 AM
This revision was automatically updated to reflect the committed changes.
nickdesaulniers reopened this revision.Jun 8 2021, 8:56 AM
This revision is now accepted and ready to land.Jun 8 2021, 8:56 AM
nickdesaulniers planned changes to this revision.Jun 8 2021, 8:56 AM
srj added a subscriber: srj.Jun 8 2021, 9:06 AM
  • fixes for MIPS
This revision is now accepted and ready to land.Jun 8 2021, 10:50 AM

I've retested with all backends enabled; all green now. Changes added to:

  • llvm/lib/Target/Mips/MipsCallLowering.cpp
  • llvm/lib/Target/Mips/MipsTargetMachine.cpp
  • llvm/test/CodeGen/Mips/stack-alignment.ll

Planning to recommit now.

This revision was landed with ongoing or failed builds.Jun 8 2021, 10:59 AM
This revision was automatically updated to reflect the committed changes.