[PATCH] D130466: [LICM] - Add option to allow data races

Thu Sep 1 00:26:47 PDT 2022

nikic added a comment.

In D130466#3761744 <https://reviews.llvm.org/D130466#3761744>, @efriedma wrote:

>> Prohibit this transformation if store points to constant memory using bool PointToConstantMemory.
>
> You can't use pointsToConstantMemory to prove the property you need.  The problem is that it doesn't do what you want if it can't prove anything.  You need a function that says the memory isn't writable unless it can prove the memory is writable (maybe call it something like "pointsToWritableMemory").
>
> Getting the proof requirements wrong is a common trap for newcomers to writing compiler optimizations; please watch out for it in the future.

In practice, would that be alloca and non-constant globals, or can we handle any other cases as well? I don't think doing this for noalias calls would be legal, because the noalias semantics don't say anything about the memory being writable, right?

================
Comment at: llvm/test/Transforms/LICM/promote-sink-store.ll:1
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt -licm -licm-force-thread-model-single -S %s | FileCheck %s
----------------
Please run the tests through `-instnamer` so the instructions have names. Also, it should be able to significantly reduce this test, it currently contains code not relevant to the transform.

You'll also want to add a negative test for the constant memory case.

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

https://reviews.llvm.org/D130466