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

[Shubham Narlawar via Phabricator via llvm-commits]

gsocshubham added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LICM.cpp:1859
+      MSSAU.insertDef(cast<MemoryDef>(NewMemAcc),
+                      AllowDataRaces ? false : true);
       // FIXME: true for safety, false may still be correct.
----------------
efriedma wrote:
> Not sure how this is related.
Removed.


================
Comment at: llvm/lib/Transforms/Scalar/LICM.cpp:2119
   if (!SafeToInsertStore) {
-    if (IsKnownThreadLocalObject)
+    if (IsKnownThreadLocalObject || AllowDataRaces)
       SafeToInsertStore = true;
----------------
efriedma wrote:
> I think you need a few more safety checks, even if you're assuming the program is single-threaded.  Just because it's legal to load from a memory location, doesn't mean it's legal to store to it.  For example, you can't write to a constant.
I am not sure about writing to a constant. Can you elaborate with an example where it would fail?


================
Comment at: llvm/test/Transforms/LICM/reg-promote.ll:17
+; CHECK-NEXT:    [[TMP7:%.*]] = alloca i32, align 4
+; CHECK-NEXT:    [[TMP8:%.*]] = alloca i32, align 4
+; CHECK-NEXT:    store ptr [[TMP0:%.*]], ptr [[TMP4]], align 8, !tbaa [[TBAA10:![0-9]+]]
----------------
efriedma wrote:
> Can you try to simplify the testcase a bit more?  All these allocas seem unnecessary to demonstrate the transform.
I used `mem2reg` to remove allocas from test but it changes the state of program which will prohibit the optimization which I am proposing.


================
Comment at: llvm/test/Transforms/LICM/reg-promote.ll:187
+!16 = distinct !{!16, !17}
+!17 = !{!"llvm.loop.mustprogress"}
----------------
gsocshubham wrote:
> TODO - Remove attributes from testcase.
Removed.


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

https://reviews.llvm.org/D130466