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

[David Li via Phabricator via llvm-commits]

davidxl marked 6 inline comments as done.
davidxl 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;
----------------
wmi wrote:
> Nit: if (!RV || !(Condition = dyn_cast<CmpInst>(RV))) return false;
ok


================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:1726
       }
+      SimplifyInstructionsInBlock(BB, TLI);
       return true;
----------------
wmi wrote:
> 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? 
This simplifies the ret instruction after the condition is removed.


================
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) {
----------------
wmi wrote:
> 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? 
Right. This can be a TODO.


================
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.
----------------
wmi wrote:
> 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.
Ok.


================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:2005
     LVI->enableDT();
   LVI->threadEdge(PredBB, BB, SuccBB);
 
----------------
efriedma wrote:
> I'm surprised threadEdge still works.  Probably want to update the documentation for that API if we really want to allow SuccBB to be null.
ok. Will document as a follow up.


================
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
----------------
wmi wrote:
> Tests need to be processed with opt -instnamer. 
ok


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

https://reviews.llvm.org/D68898