[PATCH] D49151: [SimplifyIndVar] Avoid generating truncate instructions with non-hoisted Load operand.

[Sebastian Pop via Phabricator via llvm-commits]

sebpop added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/IndVarSimplify.cpp:1400
+  unsigned ExtendOperIdx = DU.NarrowUse->getOperand(0) == NarrowDef ? 1 : 0;
+  assert(DU.NarrowUse->getOperand(1-ExtendOperIdx) == DU.NarrowDef && "bad DU");
+
----------------
clang-format


================
Comment at: llvm/lib/Transforms/Scalar/IndVarSimplify.cpp:1422
+  // Verifying that Defining operand is an AddRec
+  const SCEV *op1 = SE->getSCEV(WideDef);
+  const SCEVAddRecExpr *AddRecOp1 = dyn_cast<SCEVAddRecExpr>(op1);
----------------
Variable names should start with a Capital letter. 


================
Comment at: llvm/lib/Transforms/Scalar/IndVarSimplify.cpp:1430
+    const SCEVSignExtendExpr *AddRecOp2 = dyn_cast<SCEVSignExtendExpr>(ExtendOperExpr);
+    if (!AddRecOp2)
+      return false;
----------------
Rewrite like so:

  if (!isa<SCEVSignExtendExpr>(ExtendOperExpr))
    return false;


================
Comment at: llvm/lib/Transforms/Scalar/IndVarSimplify.cpp:1436
+    const SCEVZeroExtendExpr *AddRecOp2 = dyn_cast<SCEVZeroExtendExpr>(ExtendOperExpr);
+    if (!AddRecOp2)
+      return false;
----------------
idem: use isa<> shorter format.


================
Comment at: llvm/lib/Transforms/Scalar/IndVarSimplify.cpp:1471
+        (isa<ZExtInst>(User) && (getExtendKind(NarrowUse) == ZeroExtended))) &&
+        (User->getType() == WideType)) {
+        LLVM_DEBUG(dbgs() << "INDVARS: eliminating " << *User
----------------
Can you please remove all the unneeded parentheses?
Also I find it more clear if you first check for all cases that exit the loop with `WideningUseful = false; break;`




================
Comment at: llvm/lib/Transforms/Scalar/IndVarSimplify.cpp:1475
+        ++NumElimExt;
+        User->replaceAllUsesWith(WideBO);
+        DeadInsts.emplace_back(User);
----------------
There is a path on which we would transform the code in this rAUW stmt, and in a later iteration will fail in the `else break` clause.
Can we transform this loop such that we split the analysis phase from code generation part?  Maybe by using a vector of the things to be replaced.  The analysis part that may fail with `return false;` should be moved before `// Generating a widening use instruction.`
You can also split all the code gen part in a separate function that does not fail, and call it from below...


================
Comment at: llvm/lib/Transforms/Scalar/IndVarSimplify.cpp:1597
+    if (widenWithVariantLoadUse(DU))
+      return nullptr;
+
----------------
... and call it from here.

  if (analysisFails())
    return nullptr;
  codeGenWiden();



https://reviews.llvm.org/D49151