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

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

I believe the logic is now correct, but testing could be improved. Some suggestions:

- Use a single file with two RUN lines, only one of which has `-licm-force-thread-model-single`. Combined with `--check-prefixes`, this will make it clear in which cases behavior differs based on the flag and in which cases it is the same.
- The current test cases look too complicated. For globals, I believe a very simple loop that just loads / stores the global inside the loop should be enough. Without the flag this will lead to only load promotion, and with it to store promotion.
- We should have a variant of that test with a constant global (should have no store promotion, independent of the flag).
- We should have a variant of the test with a function argument (unknown if writable, so should have no store promotion).
- We should have a variant of the test with an alloca that is captured (store promotion legal with the flag, because capture does not impact thread safety).



================
Comment at: llvm/lib/Transforms/Scalar/LICM.cpp:1934
 bool llvm::promoteLoopAccessesToScalars(
-    const SmallSetVector<Value *, 8> &PointerMustAliases,
+    AAResults *AA, const SmallSetVector<Value *, 8> &PointerMustAliases,
     SmallVectorImpl<BasicBlock *> &ExitBlocks,
----------------
Unused AAResults argument?


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

https://reviews.llvm.org/D130466