[PATCH] D128863: Add switch to use "source_filename" instead of a hash ID for globally promoted local

[Bill Wendling via Phabricator via llvm-commits]

void added a comment.

In D128863#3698151 <https://reviews.llvm.org/D128863#3698151>, @tejohnson wrote:

> I think the summary could give a clearer description of the problem (I'd also use "module hash" vs "hash ID" to be more explicit as to what the hash is). I think instead of:
>
> During LTO a local promoted to a global gets a unique suffix based
> partially on a hash ID. This has a problem, because the hash ID is not
> the same as the original hash ID. So any tool that's validating changes
> will see a change when there isn't one (i.e. the names of the promoted
> locals are changed, but it doesn't reflect a real change).
>
> Maybe something like this?:
>
> During LTO a local promoted to a global gets a unique suffix based on
> a hash of the module IR. This means that changes in the local's module
> can affect the contents in another module that imported it (because the name
> of the imported promoted local is changed, but that doesn't reflect a
> real change in the importing module). So any tool that's
> validating changes to the importing module will see a superficial change.
>
> Instead of using the module hash, ...

Looks good to me. :-)



================
Comment at: llvm/test/ThinLTO/X86/promote-local-name.ll:20
+
+; The copy in %t1.bc should not be exported/promoted/renamed
+; RUN: llvm-lto -use-source-filename-for-promoted-locals -thinlto-action=promote %t1.bc -thinlto-index=%t3.bc -o - | llvm-dis -o - | FileCheck %s --check-prefix=NOEXPORTSTATIC
----------------
tejohnson wrote:
> void wrote:
> > tejohnson wrote:
> > > I don't think there is a need for this testing here (this is behavior tested elsewhere). You should be able to remove promote-local-name-1.ll completely from this test.
> > I removed the extra check, but when I tried to remove one of the .ll files it fails. (I low-key don't know what the `llvm-lto` tool with all of its options does.) Can I just leave it as is?
> Hmm, that's odd. If you remove that .ll file and remove references to %t1.bc I'm not sure why you would be getting failures. Looks like no symbols from that file are used elsewhere.
Ah! I was doing it wrong. Fixed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128863