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

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: lib/Target/X86/X86CondBrFolding.cpp:465
+
+  // Fix RootMBB's CmpVlaue to MBB's CmpValue to TargetMBB. Don't set Imm
+  // directly. Move MBB's stmt to here as the opcode might be different.
----------------
CmpVlaue->CmpValue


================
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));
----------------
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.


================
Comment at: lib/Target/X86/X86CondBrFolding.cpp:568
+}
+bool X86CondBrFoldingPass::runOnMachineFunction(MachineFunction &MF) {
+  const X86InstrInfo *TII;
----------------
Minor, add blank line between the functions here.


================
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
----------------
Is this comment about 29 stale even in the original code?


https://reviews.llvm.org/D46662