[PATCH] D128863: Add switch to use "source_file_name" instead of Module ID for globally promoted local

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

In D128863#3663987 <https://reviews.llvm.org/D128863#3663987>, @void wrote:

> The following goes into detail, some of which you know of course, but I wanted to be thorough.

Thanks for going into all the details. I had understood most of this before but it helped connect the dots. This was the main part I was missing:

> If it comes to pass that changing `bar()` causes it to be inlinable into `foo()`, then `foo()`'s generated code does change and we'll have to address that in some other fashion (probably with `noinline`). But that's changing the patch rather than what the compiler's doing.

So essentially, if changes percolate into other functions in sections that you are allowed to patch, you just expand the patch to include those. But if they percolate into unpatchable sections, other than the type of changes being handled here you will need to modify the code manually to prevent changes.

Unfortunately, I think that you will have limited success with that approach. E.g. by marking a function 'noinline' you will potentially block other inlines that happed for that function in the original version, creating other differences that could affect your unpatchable section (e.g. let's say bar was previously already inlined in another callsite in foo or some other function in the unpatchable section, now you have a change at that callsite from blocking all inlining of bar). There are other whole program optimizations that will be harder to adjust with manual source changes too.

I'm ok with approving this patch though if it helps you make forward progress. Can you add a test? One other suggestion in the code below too.

I do think you will run into other issues down the line that are related but not so easy to work around. One thing I can think of is to prevent unpatchable sections from taking part in ThinLTO in the first place - which today is only possible afaik if they are in separate files that can be compiled to native code without ThinLTO. It gives a performance cost but would avoid the potential patching issues due to changes of this nature.



================
Comment at: llvm/lib/Transforms/Utils/FunctionImportUtils.cpp:105
+    SmallString<256> Suffix(SGV->getParent()->getSourceFileName());
+    std::replace_if(Suffix.begin(), Suffix.end(),
+                    [&](char ch) {
----------------
Can you replace all of this with something simpler that replaces all non-alphanumeric chars, see example at https://github.com/llvm/llvm-project/blob/d71128d97df9986e98c3d2b56e5629f5e36e4c45/llvm/lib/ObjCopy/ELF/ELFObject.cpp#L1287-L1289


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128863