[PATCH] D46662: [X86] condition branches folding for three-way conditional codes

[Rong Xu via Phabricator via llvm-commits]

xur marked 10 inline comments as done.
xur added a comment.

Thanks for Craig.'s review. Here is the new patch that integrated most of his comments.
I will post another version that moves analyzeMBB() to X86CondBrFolding, as suggested by Craig.



================
Comment at: lib/Target/X86/X86CondBrFolding.cpp:414
+  MachineBasicBlock *TargetMBB = MBBInfo->TBB;
+  assert(MBBInfo);
+  BranchProbability TargetProb = MBPI->getEdgeProbability(&MBB, MBBInfo->TBB);
----------------
craig.topper wrote:
> This assert doesn't really accomplish anything. MBBInfo was already dereferenced on the line before so we crashed before we got to the assert.
moved before the deference.


================
Comment at: lib/Target/X86/X86CondBrFolding.cpp:522
+    auto MBBInfoP = llvm::make_unique<TargetMBBInfo>(MBB);
+    if (!MBBInfoP->analyzeMBB()) {
+      MBBInfos.insert(std::make_pair(&MBB, nullptr));
----------------
craig.topper wrote:
> This might just be me, but I kind of feel like analyzeMBB should be part of the X86condBrFolding and the TargetMBBInfo struct should only be created if its needed with the information the analysis collected. Right now we speculatively create a struct we have to delete if we fail the checks.
You have a good point. Current way is easier for sharing the  analysis result but the structure is not the best.
I'll restructure this in a later patch.


================
Comment at: test/CodeGen/X86/switch-bt.ll:142
+; The balanced binary switch here would start with a comparison against 40, but
 ; it is currently starting with 29 because of the density-sum heuristic.
+; CHECK: cmpl $40
----------------
craig.topper wrote:
> Is this comment about 29 stale even in the original code?
You are right! I did not pay attention to this.
I deleted the stale comments.


https://reviews.llvm.org/D46662