[PATCH] D34515: [ARM] Materialise some boolean values to avoid a branch

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:7443
+  if (isNullConstant(Carry))
+    return SDValue();
+
----------------
rogfer01 wrote:
> efriedma wrote:
> > When do we hit this case?  I would expect that normally DAGCombiner::visitADDCARRY would combine this away.
> In a testcase like
> 
> ```
> define i32 @test1a(i32 %a, i32 %b) {
> entry:
>   %cmp = icmp ne i32 %a, %b
>   %cond = zext i1 %cmp to i32
>   ret i32 %cond
> }
> ```
> without this we generate
> 
> ```
> 	subs	r0, r0, r1
> 	movs	r1, #1
> 	subs	r2, r1, #1
> 	mov	r2, r0
> 	sbcs	r2, r1
> 	sbcs	r0, r2
> 	bx	lr
> ```
> 
> with it we can generate
> 
> ```
> 	subs	r0, r0, r1
> 	subs	r1, r0, #1
> 	sbcs	r0, r1
> 	bx	lr
> ```
> 
> Alternatively I could queue a combine to the newly created `ISD::SUBCARRY` in line 12270 below. I think `DCI.CombineTo` should be able to do that. Does this sound right?
If we're not correctly adding new nodes to the DAGCombiner queue in a custom DAGCombine, we should fix that.


================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:12245
 
+  if (!VT.isInteger())
+      return SDValue();
----------------
rogfer01 wrote:
> efriedma wrote:
> > Do we generate ARMISD::CMOV for values which aren't integers?
> In cases like this 
> ```lang=c
> float a;
> int b;
> a = (b & 0x1) != 0;
> ```
> there is a moment where the DAG contains this node
> ```
>  t19: f32 = ARMISD::CMOV ConstantFP:f32<0.000000e+00>, ConstantFP:f32<1.000000e+00>, Constant:i32<1>, Register:i32 %cpsr, t18
> ```
Huh.

I guess we're getting lucky that the code below to create an AssertZext doesn't crash.


https://reviews.llvm.org/D34515