Page MenuHomePhabricator

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

Authored by nikic on Thu, Mar 16, 9:15 AM.

Details

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.

Diff Detail

Unit TestsFailed

TimeTest
260 msx64 debian > LLVM.CodeGen/RISCV::inline-asm.ll
Script: -- : 'RUN: at line 2'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/llc -mtriple=riscv32 -verify-machineinstrs -no-integrated-as < /var/lib/buildkite-agent/builds/llvm-project/llvm/test/CodeGen/RISCV/inline-asm.ll | /var/lib/buildkite-agent/builds/llvm-project/build/bin/FileCheck -check-prefix=RV32I /var/lib/buildkite-agent/builds/llvm-project/llvm/test/CodeGen/RISCV/inline-asm.ll

Event Timeline

nikic created this revision.Thu, Mar 16, 9:15 AM
nikic requested review of this revision.Thu, Mar 16, 9:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptThu, Mar 16, 9:15 AM

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 added a comment.Thu, Mar 16, 1:55 PM

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 accepted this revision.Thu, Mar 16, 2:44 PM

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.

This revision is now accepted and ready to land.Thu, Mar 16, 2:44 PM

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 added a comment.Fri, Mar 17, 8:27 AM

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.