[PATCH] Select Elimination in InstCombine

[Gerolf Hoflehner]

> hfinkel: 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?
I guess I had said that compile-time is not impacted, but never showed any data. And then I had said many more things about compile-time measurements, our test suite and dominators, so the noise numbers should come as no surprise (all with 0.1-0.2 secs on x86):

Performance Regressions - Compile Time	Δ 	Previous	Current	σ 	Δ (B)	σ (B)
SingleSource/UnitTests/ObjC++/property-reference <http://ghoflehner.apple.com/perf/v4/nts/2/graph?test.202=2>	4.37%	0.4192	0.4375	0.0003	0.00%	0.0003
SingleSource/Benchmarks/Shootout-C++/fibo <http://ghoflehner.apple.com/perf/v4/nts/2/graph?test.255=2>	3.98%	0.2714	0.2822	0.0005	0.00%	0.0005
MultiSource/Applications/lemon/lemon <http://ghoflehner.apple.com/perf/v4/nts/2/graph?test.521=2>	3.35%	0.5018	0.5186	0.0045	0.00%	0.0045
SingleSource/UnitTests/ObjC++/property-reference-object <http://ghoflehner.apple.com/perf/v4/nts/2/graph?test.62=2>	3.05%	0.3375	0.3478	0.0002	0.00%	0.0002
MultiSource/Applications/sqlite3/sqlite3 <http://ghoflehner.apple.com/perf/v4/nts/2/graph?test.513=2>	2.81%	7.1620	7.3631	0.0438	0.00%	0.0438
External/SPEC/CFP2006/444_namd/444_namd <http://ghoflehner.apple.com/perf/v4/nts/2/graph?test.163=2>	2.15%	3.1007	3.1675	0.0021	0.00%	0.0021
MultiSource/Applications/oggenc/oggenc <http://ghoflehner.apple.com/perf/v4/nts/2/graph?test.537=2>	1.73%	2.0891	2.1252	0.0065	0.00%	0.0065
External/SPEC/CINT2000/197_parser/197_parser <http://ghoflehner.apple.com/perf/v4/nts/2/graph?test.447=2>	1.63%	1.1629	1.1819	0.0000	0.00%	0.0000
External/SPEC/CINT2000/176_gcc/176_gcc <http://ghoflehner.apple.com/perf/v4/nts/2/graph?test.333=2>	1.60%	12.4641	12.6631	0.0030	0.00%	0.0030
External/SPEC/CINT95/099_go/099_go <http://ghoflehner.apple.com/perf/v4/nts/2/graph?test.528=2>	1.51%	1.7386	1.7649	0.0020	0.00%	0.0020
External/SPEC/CINT2006/403_gcc/403_gcc <http://ghoflehner.apple.com/perf/v4/nts/2/graph?test.59=2>	1.46%	30.0626	30.5019	0.0195	0.00%	0.0195
External/SPEC/CINT95/134_perl/134_perl <http://ghoflehner.apple.com/perf/v4/nts/2/graph?test.319=2>	1.34%	2.5077	2.5413	0.0010	0.00%	0.0010
External/SPEC/CINT2006/400_perlbench/400_perlbench <http://ghoflehner.apple.com/perf/v4/nts/2/graph?test.280=2>	1.31%	10.8686	11.0114	0.0005	0.00%	0.0005
External/SPEC/CINT2006/445_gobmk/445_gobmk <http://ghoflehner.apple.com/perf/v4/nts/2/graph?test.272=2>	1.18%	7.7186	7.8093	0.0010	0.00%	0.0010
External/SPEC/CINT2000/253_perlbmk/253_perlbmk <http://ghoflehner.apple.com/perf/v4/nts/2/graph?test.241=2>	1.11%	5.5141	5.5752	0.0047	0.00%	0.0047
External/SPEC/CINT2000/254_gap/254_gap <http://ghoflehner.apple.com/perf/v4/nts/2/graph?test.99=2>	1.03%	4.5858	4.6332	0.0058	0.00%	0.0058

> On Sep 28, 2014, at 9:00 AM, hfinkel at anl.gov wrote:
> 
> ================
> 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
> 
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140929/09fc306b/attachment.html>