[PATCH] D29620: [JumpThreading] Thread through guards

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev added inline comments.


================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:2109
+  // Duplicate all instructions before the guard in the unguarded branch.
+  UnguardedBlock =
+      DuplicateInstructionsInSplitBetween(BB, UnguardedBlock, Guard,
----------------
sanjoy wrote:
> Where is `BBDupThreshold` defined?  That needs to be a `cl::opt`.
It already is, see line 53

  static cl::opt<unsigned>
  BBDuplicateThreshold("jump-threading-threshold",
            cl::desc("Max block size to duplicate for jump threading"),
            cl::init(6), cl::Hidden);

And then line 67

  BBDupThreshold = (T == -1) ? BBDuplicateThreshold : unsigned(T);

It is used in getJumpThreadDuplicationCost calculation. Actually this function does pretty much what we want. I reused it to avoid duplication of its logic.


================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:2118
+  GuardedBlock =
+      DuplicateInstructionsInSplitBetween(BB, GuardedBlock, AfterGuard,
+                                          BBDupThreshold, GuardedMapping);
----------------
sanjoy wrote:
> Right now it is possible for this transform to bail out after cloning some instructions if the first `DuplicateInstructionsInSplitBetween` passes but the second one fails.  I'd restructure this a little differently:  I'd first clone `GuardedBlock` and then `UnguardedBlock`; with an assert checking that if cloning `GuardedBlock` was successful, so was `UnguardedBlock`, since the latter clones fewer instructions.
I hoisted instruction count check out of the method and only invoke it for guarded branch.


================
Comment at: lib/Transforms/Utils/CloneFunction.cpp:765
+    Instruction *Inst = &*BI;
+    if (dyn_cast<PHINode>(BI))
+      continue;
----------------
sanjoy wrote:
> Use `isa<>` here.
This code is no more, replaced with calculation function call.


https://reviews.llvm.org/D29620