[PATCH] D97971: [IPSCCP] don't propagate constant in section when caller/callee sections mismatch

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

In D97971#2607973 <https://reviews.llvm.org/D97971#2607973>, @nickdesaulniers wrote:

> In D97971#2604415 <https://reviews.llvm.org/D97971#2604415>, @lebedev.ri wrote:
>
>> Does langref's `section` actually provide such guarantees?
>
> Added a note, PTAL.
>
> In D97971#2604727 <https://reviews.llvm.org/D97971#2604727>, @jdoerfert wrote:
>
>> If we do this, we should create a helper somewhere that tells you if you can forward a constant, given an (Abstract)CallSite. For example, thread local constants can't be forwarded across callbacks.
>> So it's not only that we (might) have multiple things we need to filter but also multiple locations, Attributor needs this as well.
>
> Yep, that sounds feasible, and with my rewrite is fairly straightforward now that it's decoupled from `SCCSolver`.  For the case you describe, does it also pertain to passing arguments or no?  While my current implementation is tailed to the existing argument walk on the `CallBase`, that could simply be done for such a helper function you describe.  Do you have a recommendation on a good location to place such a helper so that `SCCSolver` and `Attributor` could both refer to it?
>
> In D97971#2604785 <https://reviews.llvm.org/D97971#2604785>, @MaskRay wrote:
>
>>> `void callee (int* x) { int y = *z; }`
>>
>> Should it be `void callee (int* x) { int y = z; }` ?
>
> Thanks, fixed.
>
>>> Later, Dead Argument Elimination (deadargelim) may even change the signature of callee removing dead arguments from its signature
>>
>> Assuming `void callee` does not have the `static` keyword. deadargelim applies on local linkage functions.
>> LTO may internalize functions if the linker knows `callee` does not need to be exported. Yes, deadargelim can happen in that case.
>
> Fixed the description to note that `callee` is `static`.
>
>> https://github.com/ClangBuiltLinux/linux/issues/1301
>>
>>> `WARNING: modpost: vmlinux.o(.text+0x68943): Section mismatch in reference from the function __nodes_weight() to the variable .init.data:numa_nodes_parsed`
>>
>> Is the warning about a non-`.init.text` function accesses `.init.data` data?
>
> Yes.  That's a potential use after free, since `.init.data` data will be unmapped (and potentially remapped) after initialization, while the non-`.init.text` (ie. just `.text) function is not.
>
>> This feels to me that the kernel is asking too much.
>
> Perhaps; do you have a recommendation on how best to pass references to global data in different sections otherwise?  I'll note the use of this pattern of `__init` and `__init_data` is so pervasive throughout the sources, that "don't do that" is a non-starter. But perhaps there's another approach, like storing the constant to a pointer or something, or some other attributes you mention below?
>
>> If we rename `.init.text` to `.text.hot.` and `.init.data` to `.data.hot.`, this rule would mean that a regular `.text` function cannot access `.data.hot.` data.
>
> If we rename `.init.text` and `.init.data`, it will leak memory in the kernel as none of the annotated sections will get cleaned up.
>
>> This does not sound right to me.
>> If the kernel wants to have an optimization barrier on the arguments, it should use some attributes if it does not want to use `alwaysinline`.
>
> Are there existing attributes we can use on parameters or arguments?

Hope @jdoerfert can shed some light on the attribute usage here...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97971