[PATCH] D68898: JumpThreading: enhance JT to handle BB with no successor and address comparison

[Wei Mi via Phabricator via llvm-commits]

wmi added inline comments.


================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:1076-1081
+    if (!RV)
+      return false;
+    CmpInst *Cmp = dyn_cast<CmpInst>(RV);
+    if (!Cmp)
+      return false;
+    Condition = Cmp;
----------------
Nit: if (!RV || !(Condition = dyn_cast<CmpInst>(RV))) return false;


================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:1726
       }
+      SimplifyInstructionsInBlock(BB, TLI);
       return true;
----------------
For BB being handled in this patch (containing Ret instruction), OnlyDest is always nullptr so this block will not be executed for it. Is it for other purpose? 


================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:1769-1780
+    LLVM_DEBUG(auto *TI = BB->getTerminator();
+               assert(isa<ReturnInst>(TI)););
+    auto *Pred = PredToDestList.begin()->first;
+    PredsToFactor.push_back(Pred);
+    // We need to find the pred val associated with the selected pred:
+    for (const auto &PredValue : PredValues) {
+      if (PredValue.second == Pred) {
----------------
For BB with Ret instruction, we don't factor predecessors with the same PredVal here. If we factor predecessors with the same PredVal, we may have less clones? 


================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:1946-1948
 /// ThreadEdge - We have decided that it is safe and profitable to factor the
 /// blocks in PredBBs to one predecessor, then thread an edge from it to SuccBB
 /// across BB.  Transform the IR to reflect this change.
----------------
It will be helpful to add some comment for the case that SuccBB is nullptr and maybe a TODO for the extension and refactoring needed.


================
Comment at: test/Transforms/JumpThreading/addr.ll:15-16
+2:                                                ; preds = %1
+  %3 = icmp eq i32 %0, 17
+  %4 = select i1 %3, i32* getelementptr inbounds (%"struct.std::array", %"struct.std::array"* @_ZL1x, i64 0, i32 0, i64 2), i32* getelementptr inbounds (%"struct.std::array", %"struct.std::array"* @_ZL1x, i64 1, i32 0, i64 0)
+  br label %6
----------------
Tests need to be processed with opt -instnamer. 


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

https://reviews.llvm.org/D68898