[llvm] 388b684 - [LICM] Separate check for writability and thread-safety (NFCI)

[Nikita Popov via llvm-commits]

Author: Nikita Popov
Date: 2022-09-05T09:43:17+02:00
New Revision: 388b684354cc71bd4043ddccbcfd91fb338d8b1e

URL: https://github.com/llvm/llvm-project/commit/388b684354cc71bd4043ddccbcfd91fb338d8b1e
DIFF: https://github.com/llvm/llvm-project/commit/388b684354cc71bd4043ddccbcfd91fb338d8b1e.diff

LOG: [LICM] Separate check for writability and thread-safety (NFCI)

This used a single check to make sure that the object is both
writable and thread-local. Separate them out to make the
deficiencies in the current code more obvious.

Added: 
    

Modified: 
    llvm/lib/Transforms/Scalar/LICM.cpp

Removed: 
    


################################################################################
diff  --git a/llvm/lib/Transforms/Scalar/LICM.cpp b/llvm/lib/Transforms/Scalar/LICM.cpp
index 9d98beaa19e66..29e14d8dfe0b4 100644
--- a/llvm/lib/Transforms/Scalar/LICM.cpp
+++ b/llvm/lib/Transforms/Scalar/LICM.cpp
@@ -1890,6 +1890,29 @@ bool isNotVisibleOnUnwindInLoop(const Value *Object, const Loop *L,
          isNotCapturedBeforeOrInLoop(Object, L, DT);
 }
 
+bool isWritableObject(const Value *Object) {
+  // TODO: Alloca might not be writable after its lifetime ends.
+  // See https://github.com/llvm/llvm-project/issues/51838.
+  if (isa<AllocaInst>(Object))
+    return true;
+
+  // TODO: Also handle sret.
+  if (auto *A = dyn_cast<Argument>(Object))
+    return A->hasByValAttr();
+
+  // TODO: Noalias has nothing to do with writability, this should check for
+  // an allocator function.
+  return isNoAliasCall(Object);
+}
+
+bool isThreadLocalObject(const Value *Object, const Loop *L,
+                         DominatorTree *DT) {
+  // The object must be function-local to start with, and then not captured
+  // before/in the loop.
+  return isIdentifiedFunctionLocal(Object) &&
+         isNotCapturedBeforeOrInLoop(Object, L, DT);
+}
+
 } // namespace
 
 /// Try to promote memory values to scalars by sinking stores out of the
@@ -2109,14 +2132,12 @@ bool llvm::promoteLoopAccessesToScalars(
   }
 
   // We know we can hoist the load, but don't have a guaranteed store.
-  // Check whether the location is thread-local. If it is, then we can insert
-  // stores along paths which originally didn't have them without violating the
-  // memory model.
+  // Check whether the location is writable and thread-local. If it is, then we
+  // can insert stores along paths which originally didn't have them without
+  // violating the memory model.
   if (StoreSafety == StoreSafetyUnknown) {
     Value *Object = getUnderlyingObject(SomePtr);
-    if ((isNoAliasCall(Object) || isa<AllocaInst>(Object) ||
-         (isa<Argument>(Object) && cast<Argument>(Object)->hasByValAttr())) &&
-        isNotCapturedBeforeOrInLoop(Object, CurLoop, DT))
+    if (isWritableObject(Object) && isThreadLocalObject(Object, CurLoop, DT))
       StoreSafety = StoreSafe;
   }