[PATCH] D24168: Refactor LICM pass in preparation for LoopSink pass.

[Chandler Carruth via llvm-commits]

chandlerc added inline comments.

================
Comment at: lib/Transforms/Scalar/LICM.cpp:445-451
@@ +444,9 @@
+                        LoopSafetyInfo *SafetyInfo) {
+  if (!isa<LoadInst>(I) && !isa<CallInst>(I) &&
+      !isa<BinaryOperator>(I) && !isa<CastInst>(I) && !isa<SelectInst>(I) &&
+      !isa<GetElementPtrInst>(I) && !isa<CmpInst>(I) &&
+      !isa<InsertElementInst>(I) && !isa<ExtractElementInst>(I) &&
+      !isa<ShuffleVectorInst>(I) && !isa<ExtractValueInst>(I) &&
+      !isa<InsertValueInst>(I))
+    return false;
+
----------------
Why this change?

We try to avoid testing isa<T> just before we do a dyn_cast<T> because it duplicates work. So the other arrangement is in many ways much more common.

If you're worried about the performance of this code or just trying to make it more readable what goes where, there are perhaps better patterns that can be used, but I'm not sure which to suggest without understanding the motivation.

================
Comment at: lib/Transforms/Scalar/LICM.cpp:468
@@ -459,3 +467,3 @@
     if (LI->getType()->isSized())
-      Size = I.getModule()->getDataLayout().getTypeStoreSize(LI->getType());
+      Size = LI->getModule()->getDataLayout().getTypeStoreSize(LI->getType());
 
----------------
Any particular reason to make this change? It's not necessarily a bad change, but it seems surprising here.

================
Comment at: lib/Transforms/Scalar/LICM.cpp:517-523
@@ -509,14 +516,9 @@
 
-  // Only these instructions are hoistable/sinkable.
-  if (!isa<BinaryOperator>(I) && !isa<CastInst>(I) && !isa<SelectInst>(I) &&
-      !isa<GetElementPtrInst>(I) && !isa<CmpInst>(I) &&
-      !isa<InsertElementInst>(I) && !isa<ExtractElementInst>(I) &&
-      !isa<ShuffleVectorInst>(I) && !isa<ExtractValueInst>(I) &&
-      !isa<InsertValueInst>(I))
-    return false;
-
-  // TODO: Plumb the context instruction through to make hoisting and sinking
-  // more powerful. Hoisting of loads already works due to the special casing
-  // above.
-  return isSafeToExecuteUnconditionally(I, DT, CurLoop, SafetyInfo, nullptr);
+  if (SafetyInfo)
+    // TODO: Plumb the context instruction through to make hoisting and sinking
+    // more powerful. Hoisting of loads already works due to the special casing
+    // above.
+    return isSafeToExecuteUnconditionally(I, DT, CurLoop, SafetyInfo, nullptr);
+  else
+    return true;
 }
----------------
Please prefer early return:

  if (!SafetyInfo)
    return true;

  // ...
  return isSafeTo...



https://reviews.llvm.org/D24168