[PATCH] Select Elimination in InstCombine
hfinkel at anl.gov
hfinkel at anl.gov
Sun Sep 28 09:00:28 PDT 2014
================
Comment at: lib/Transforms/InstCombine/InstCombine.h:247
@@ -244,2 +246,3 @@
Instruction *visitInstruction(Instruction &I) { return nullptr; }
+ bool dominatesAllUses(const Instruction *DI, const Instruction *UI, const BasicBlock *DB) const;
----------------
80 cols.
================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:2428
@@ -2427,1 +2427,3 @@
+/// \brief Check that one use is in the same block as the defintion and all
+/// other uses are in blocks dominated by a given block
----------------
This is not quite right, the function actually asserts if the definition and the provided use are in different blocks (it does not check this condition).
================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:2458
@@ +2457,3 @@
+ const BasicBlock *BB = SI->getParent();
+ if (!BB)
+ return false;
----------------
Can this really be called on un-inserted instructions?
================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:2461
@@ +2460,3 @@
+ auto *BI = dyn_cast_or_null<BranchInst>(BB->getTerminator());
+ if (!BI || BI->getNumSuccessors() != 2)
+ return false;
----------------
Or on instructions in incomplete blocks?
================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:2975
@@ +2974,3 @@
+ ReplaceWithOpd = 1;
+ if (ConstantInt *CI = dyn_cast_or_null<ConstantInt>(Op1))
+ // When the constant operand of the select and cmp
----------------
Make this an else if?
================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:2988
@@ +2987,3 @@
+ ReplaceWithOpd = 2;
+ if (ReplaceWithOpd && I.isEquality()) {
+ // Replace select with operand on else path for EQ compares.
----------------
As we only actually perform the transform if I.isEquality(), can you host that part of the check up to be with the isChainSelectCmpBranch(SI) check?
================
Comment at: lib/Transforms/InstCombine/InstructionCombining.cpp:100
@@ -98,1 +99,3 @@
AU.addRequired<TargetLibraryInfo>();
+ AU.addRequired<DominatorTreeWrapperPass>();
+ AU.addPreserved<DominatorTreeWrapperPass>();
----------------
Are you going to separate this into a follow-up patch, or are you planning to provide compile-time measurements to before committing this one?
http://reviews.llvm.org/D5258
More information about the llvm-commits
mailing list