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

Mon Jul 25 12:35:04 PDT 2022

efriedma added a comment.

I don't really want to introduce an option named "allow-data-races".

If we want to do something based on the assumption the program is single-threaded, the option should be named that.  And ideally, it shouldn't be specific to LICM; it should be something any pass can query from TargetTransformInfo.

================
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.
----------------
Not sure how this is related.

================
Comment at: llvm/lib/Transforms/Scalar/LICM.cpp:2119
   if (!SafeToInsertStore) {
-    if (IsKnownThreadLocalObject)
+    if (IsKnownThreadLocalObject || AllowDataRaces)
       SafeToInsertStore = true;
----------------
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.

================
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]+]]
----------------
Can you try to simplify the testcase a bit more?  All these allocas seem unnecessary to demonstrate the transform.

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

https://reviews.llvm.org/D130466