[PATCH] D103048: [IR] make -stack-alignment= into a module attr

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson accepted this revision.
tejohnson added a comment.
This revision is now accepted and ready to land.

lgtm



================
Comment at: llvm/test/Linker/stack-alignment.ll:11
+;--- main.ll
+; NONE: error: linking module flags 'override-stack-alignment': IDs have conflicting values
+; CHECK-16: error: linking module flags 'override-stack-alignment': IDs have conflicting values
----------------
nickdesaulniers wrote:
> tejohnson wrote:
> > Will you get this error currently? I thought per comment and behavior of Error that it shouldn't give an error.
> 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.
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103048