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

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers added a comment.

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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97971