[LICM] Don't promote store to global even in single-thread mode

[nikic]

Summary

Even if there are no thread-safety concerns, we should not promote (not guaranteed-to-execute) stores to globals: While the global may be writable, we may not have provenance to perform the write. The @promote_global_noalias test case illustrates a miscompile in the presence of a noalias pointer to the global.

Worth noting that the load-only promotion may also not be well-defined depending on precise semantics (we don't specify whether load violating noalias is poison or UB -- though I believe the general inclination is to make it poison, and only stores UB), but that's a more general issue.

This is inspired by https://github.com/llvm/llvm-project/issues/60860, which is a related issue with TBAA metadata.

[jdoerfert]

Can we make this less drastic by checking the global. If it is internal and doesn't have its address taken there are no aliasing pointers, I think/hope.

[nikic]

In D146233#4200306, @jdoerfert wrote:

Can we make this less drastic by checking the global. If it is internal and doesn't have its address taken there are no aliasing pointers, I think/hope.

I don't think there's any simple way to do this. Note that this does not actually require the address to be taken -- creating a noalias pointer does not constitute a capture.

It's probably worth pointing out that this code was only recently introduced by D130466 and only applies under -mthread-model=single. This change does not affect/regress normal LICM.

[jdoerfert]

In D146233#4200610, @nikic wrote:

In D146233#4200306, @jdoerfert wrote:

Can we make this less drastic by checking the global. If it is internal and doesn't have its address taken there are no aliasing pointers, I think/hope.

I don't think there's any simple way to do this. Note that this does not actually require the address to be taken -- creating a noalias pointer does not constitute a capture.

It's probably worth pointing out that this code was only recently introduced by D130466 and only applies under -mthread-model=single. This change does not affect/regress normal LICM.

I didn't realize the latter. I was thinking we could look at all uses, >50% of the time we probably only see loads, stores, and mem* intrinsic uses. That said, there is no need only for some non-default use case.

[efriedma]

Is the issue here actually specific to global variables? I mean, you can't mark a local variable noalias, but noalias/TBAA metadata can apply to local variables.

[nikic]

In D146233#4200982, @efriedma wrote:

Is the issue here actually specific to global variables? I mean, you can't mark a local variable noalias, but noalias/TBAA metadata can apply to local variables.

There is also an issue with AA metadata, but I think it's a bit different from this one, see https://github.com/llvm/llvm-project/issues/60860#issuecomment-1474006326. In addition to that, we also currently preserve AA metadata on the promoted loads and stores in some cases where it isn't legal.