[DAGCombiner] Expand combining of FP logical operations to sign-setting FP operations

[nemanjai]

Summary

This patch does two things:

1. Expand the DAG combiner to handle vectors when combining FP logical ops and to produce negated absolute value
2. Enable this on PPC

I tried to split this out to two patches to handle the target independent part separately from the PPC part, however I'm not sure how to write a test case for it without a target that enables this on both scalar and vector types. I'd be happy to split it if someone has an idea on how to produce a test case.

I would appreciate it if target owners can try out this patch as it of course has the potential to cause infinite loops if they have combines that will convert ISD::FABS and ISD::FNEG into bitcasts and logical ops and their target is set to allow these combines for the respective type.

[nemanjai]

Ping. Do any other targets care about this? The sign manipulation instructions are definitely faster on PPC.

[lebedev.ri]

test/CodeGen/PowerPC/float-logic-ops.ll

4 ↗

(On Diff #138649)

Separate by newline

35 ↗

(On Diff #138649)

Would be nice to at least one test with one undef element, and with bad non-splat constant.

[RKSimon]

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

8933 ↗

(On Diff #138649)

We have ISD::isConstOrConstSplat - can we use that instead and merge these 2 paths?

[efriedma]

AArch64 doesn't have a nabs instruction; orr is probably faster than fabs+fneg (unless some implementations have a transition penalty between int SIMD and float SIMD). But we could accept fneg(fabs(x)) as the canonical form and pattern-match it.

[nemanjai]

In D44548#1116753, @efriedma wrote:

AArch64 doesn't have a nabs instruction; orr is probably faster than fabs+fneg (unless some implementations have a transition penalty between int SIMD and float SIMD). But we could accept fneg(fabs(x)) as the canonical form and pattern-match it.

If you think it's worthwhile, I could make hasBitPreservingFPLogic() return an enum with None, FabsOnly, FnegOnly, SingleNodeOnly, All or something along those lines... and then use that to guide which transformations to actually apply.

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

8933 ↗

(On Diff #138649)

I think that will fail if some of the elements are undef. And that seems unnecessary. Something like this:

%conv = bitcast <4 x float> %a to <4 x i32>
%and = and <4 x i32> %conv, <i32 2147483647, i32 undef, i32 undef, i32 2147483647>

should be perfectly fine to transform.

[nemanjai]

Updated the test to include invalid (non-splat) values as well as undefs as part of the splat.

[RKSimon]

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

8933 ↗

(On Diff #138649)

Its not out of the question to add an 'allowUndef' optional argument (default to false) to ISD::isConstOrConstSplat

[efriedma]

Thinking about it a bit more, we probably want to canonicalize to fabs+fneg anyway on AArch64, rather than the other way, because the fneg could combine with a later operation. And pattern-matching it is pretty easy, anyway.

[nemanjai]

Since there seems to be overall agreement that this is the direction we want to go in, here's an update that removes the unsightly dual path for scalars and vectors.
Sorry about the delay in producing this update.

[spatel]

I'm trying to improve vector-demanded-elements folds in D52912 which means we need to do better at recognizing patterns with undefs.

So I extracted 2 parts of this patch in:
rL343865
rL343940

So rebasing should allow you to shrink this patch.

[nemanjai]

Rebased to account for Sanjay's patches.

[spatel]

How about enabling the hook for PPC before adding to the generic DAG combine...
Right now, you'll get something horrible for normal fabs (and fneg?) without this patch, right?

	xscvdpspn 0, 1
	xxsldwi 0, 0, 0, 3
	mfvsrwz 3, 0
	rlwinm 3, 3, 0, 1, 31
	mtvsrd 0, 3
	xxsldwi 0, 0, 0, 1
	xscvspdpn 1, 0

[nemanjai]

In D44548#1258889, @spatel wrote:

How about enabling the hook for PPC before adding to the generic DAG combine...
Right now, you'll get something horrible for normal fabs (and fneg?) without this patch, right?

	xscvdpspn 0, 1
	xxsldwi 0, 0, 0, 3
	mfvsrwz 3, 0
	rlwinm 3, 3, 0, 1, 31
	mtvsrd 0, 3
	xxsldwi 0, 0, 0, 1
	xscvspdpn 1, 0

Yes. Although that is the worst case (since PPC f32 isn't 32 bits in a register).
No doubt, the PPC-specific portion of this needs to go in and can certainly go in first. I can commit this as the PPC portion first (with the fnabs test missing). And then the generic part (adding the fnabs test).
The question is do I need to split this into two patches and post for review or can I just commit it as two separate patches (or maybe just commit the PPC-portion and post the generic portion for review)?

[spatel]

In D44548#1259015, @nemanjai wrote:

In D44548#1258889, @spatel wrote:

How about enabling the hook for PPC before adding to the generic DAG combine...
Right now, you'll get something horrible for normal fabs (and fneg?) without this patch, right?

	xscvdpspn 0, 1
	xxsldwi 0, 0, 0, 3
	mfvsrwz 3, 0
	rlwinm 3, 3, 0, 1, 31
	mtvsrd 0, 3
	xxsldwi 0, 0, 0, 1
	xscvspdpn 1, 0

Yes. Although that is the worst case (since PPC f32 isn't 32 bits in a register).
No doubt, the PPC-specific portion of this needs to go in and can certainly go in first. I can commit this as the PPC portion first (with the fnabs test missing). And then the generic part (adding the fnabs test).
The question is do I need to split this into two patches and post for review or can I just commit it as two separate patches (or maybe just commit the PPC-portion and post the generic portion for review)?

Please extract and commit the PPC change. That's a pure win in all cases AFAICT.
Then we can come back to the fnabs part here. I don't think there will be any controversy based on the earlier comments, but let me add a couple of non-PPC tests, so we're sure about that.

[nemanjai]

Update to account for already committed PPC-specific changes.

[spatel]

LGTM

Nit: it would be nicer to check in the PPC tests with baseline asm as a preliminary step. That way, we'd just see the asm diff there too.

AArch64 and x86 don't have vector diffs, but for different reasons.

AArch:

bool hasBitPreservingFPLogic(EVT VT) const override {
  // FIXME: Is this always true? It should be true for vectors at least.
  return VT == MVT::f32 || VT == MVT::f64;
}

x86:
We're already producing the optimal 'or' instruction, and either that transform has already occurred, or this transform fires and gets folded to the same thing (I didn't step through the debug).

llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

 #define DEBUG_TYPE "dagcombine"
 
 STATISTIC(NodesCombined   , "Number of dag nodes combined");
 STATISTIC(PreIndexedNodes , "Number of pre-indexed nodes created");
 STATISTIC(PostIndexedNodes, "Number of post-indexed nodes created");
 STATISTIC(OpsNarrowed     , "Number of load/op/store narrowed");
 STATISTIC(LdStFP2Int      , "Number of fp load/store pairs transformed to int");
 STATISTIC(SlicedLoads, "Number of load sliced");
+STATISTIC(NumFPLogicOpsConv, "Number of logic ops converted to fp ops");
 
 static cl::opt<bool>
 CombinerGlobalAA("combiner-global-alias-analysis", cl::Hidden,
                  cl::desc("Enable DAG combiner's use of IR alias analysis"));
 
 static cl::opt<bool>
 UseTBAA("combiner-use-tbaa", cl::Hidden, cl::init(true),
         cl::desc("Enable DAG combiner's use of TBAA"));
static SDValue foldBitcastedFPLogic(SDNode *N, SelectionDAG &DAG,
   case ISD::AND:
     FPOpcode = ISD::FABS;
     SignMask = ~APInt::getSignMask(SourceVT.getScalarSizeInBits());
     break;
   case ISD::XOR:
     FPOpcode = ISD::FNEG;
     SignMask = APInt::getSignMask(SourceVT.getScalarSizeInBits());
     break;
-  // TODO: ISD::OR --> ISD::FNABS?
+  case ISD::OR:
+    FPOpcode = ISD::FABS;
+    SignMask = APInt::getSignMask(SourceVT.getScalarSizeInBits());
+    break;
   default:
     return SDValue();
   }
 
   // Fold (bitcast int (and (bitcast fp X to int), 0x7fff...) to fp) -> fabs X
   // Fold (bitcast int (xor (bitcast fp X to int), 0x8000...) to fp) -> fneg X
+  // Fold (bitcast int (or (bitcast fp X to int), 0x8000...) to fp) ->
+  //   fneg (fabs X)
   SDValue LogicOp0 = N0.getOperand(0);
   ConstantSDNode *LogicOp1 = isConstOrConstSplat(N0.getOperand(1), true);
   if (LogicOp1 && LogicOp1->getAPIntValue() == SignMask &&
       LogicOp0.getOpcode() == ISD::BITCAST &&
-      LogicOp0.getOperand(0).getValueType() == VT)
-    return DAG.getNode(FPOpcode, SDLoc(N), VT, LogicOp0.getOperand(0));
+      LogicOp0.getOperand(0).getValueType() == VT) {
+    SDValue FPOp = DAG.getNode(FPOpcode, SDLoc(N), VT, LogicOp0.getOperand(0));
+    NumFPLogicOpsConv++;
+    if (N0.getOpcode() == ISD::OR)
+      return DAG.getNode(ISD::FNEG, SDLoc(N), VT, FPOp);
+    return FPOp;
+  }
 
   return SDValue();
 }
 
 SDValue DAGCombiner::visitBITCAST(SDNode *N) {
   SDValue N0 = N->getOperand(0);
   EVT VT = N->getValueType(0);
 

llvm/trunk/test/CodeGen/AArch64/fabs.ll

; CHECK-NEXT:    ret
   %sub = fsub nnan float -0.0, %x
   %cond = select i1 %cmp, float %x, float %sub
   ret float %cond
 }
 
 define float @nabsf(float %a) {
 ; CHECK-LABEL: nabsf:
 ; CHECK:       // %bb.0:
-; CHECK-NEXT:    fmov w8, s0
-; CHECK-NEXT:    orr w8, w8, #0x80000000
-; CHECK-NEXT:    fmov s0, w8
+; CHECK-NEXT:    fabs s0, s0
+; CHECK-NEXT:    fneg s0, s0
 ; CHECK-NEXT:    ret
   %conv = bitcast float %a to i32
   %and = or i32 %conv, -2147483648
   %conv1 = bitcast i32 %and to float
   ret float %conv1
 }
 
 define double @nabsd(double %a) {
 ; CHECK-LABEL: nabsd:
 ; CHECK:       // %bb.0:
-; CHECK-NEXT:    fmov x8, d0
-; CHECK-NEXT:    orr x8, x8, #0x8000000000000000
-; CHECK-NEXT:    fmov d0, x8
+; CHECK-NEXT:    fabs d0, d0
+; CHECK-NEXT:    fneg d0, d0
 ; CHECK-NEXT:    ret
   %conv = bitcast double %a to i64
   %and = or i64 %conv, -9223372036854775808
   %conv1 = bitcast i64 %and to double
   ret double %conv1
 }
 
 define <4 x float> @nabsv4f32(<4 x float> %a) {

llvm/trunk/test/CodeGen/PowerPC/float-logic-ops.ll

entry:
   %conv = bitcast <2 x double> %a to <2 x i64>
   %and = xor <2 x i64> %conv, <i64 -9223372036854775808, i64 -9223372036854775808>
   %conv1 = bitcast <2 x i64> %and to <2 x double>
   ret <2 x double> %conv1
 }
 define float @nabsf(float %a) {
 ; CHECK-LABEL: nabsf:
 ; CHECK:       # %bb.0: # %entry
-; CHECK-NEXT:    xscvdpspn vs0, f1
-; CHECK-NEXT:    xxsldwi vs0, vs0, vs0, 3
-; CHECK-NEXT:    mfvsrwz r3, f0
-; CHECK-NEXT:    oris r3, r3, 32768
-; CHECK-NEXT:    mtvsrd f0, r3
-; CHECK-NEXT:    xxsldwi vs0, vs0, vs0, 1
-; CHECK-NEXT:    xscvspdpn f1, vs0
+; CHECK-NEXT:    fnabs f1, f1
 ; CHECK-NEXT:    blr
 entry:
   %conv = bitcast float %a to i32
   %and = or i32 %conv, -2147483648
   %conv1 = bitcast i32 %and to float
   ret float %conv1
 }
 
 define double @nabsd(double %a) {
 ; CHECK-LABEL: nabsd:
 ; CHECK:       # %bb.0: # %entry
-; CHECK-NEXT:    li r3, 1
-; CHECK-NEXT:    mffprd r4, f1
-; CHECK-NEXT:    sldi r3, r3, 63
-; CHECK-NEXT:    or r3, r4, r3
-; CHECK-NEXT:    mtvsrd f1, r3
+; CHECK-NEXT:    xsnabsdp f1, f1
 ; CHECK-NEXT:    blr
 entry:
   %conv = bitcast double %a to i64
   %and = or i64 %conv, -9223372036854775808
   %conv1 = bitcast i64 %and to double
   ret double %conv1
 }
 
 define <4 x float> @nabsv4f32(<4 x float> %a) {
 ; CHECK-LABEL: nabsv4f32:
 ; CHECK:       # %bb.0: # %entry
-; CHECK-NEXT:    vspltisb v3, -1
-; CHECK-NEXT:    vslw v3, v3, v3
-; CHECK-NEXT:    xxlor vs34, vs34, vs35
+; CHECK-NEXT:    xvnabssp vs34, vs34
 ; CHECK-NEXT:    blr
 entry:
   %conv = bitcast <4 x float> %a to <4 x i32>
   %and = or <4 x i32> %conv, <i32 -2147483648, i32 -2147483648, i32 -2147483648, i32 -2147483648>
   %conv1 = bitcast <4 x i32> %and to <4 x float>
   ret <4 x float> %conv1
 }
 
 define <2 x double> @nabsv2d64(<2 x double> %a) {
 ; CHECK-LABEL: nabsv2d64:
 ; CHECK:       # %bb.0: # %entry
-; CHECK-NEXT:    addis r3, r2, .LCPI13_0@toc@ha
-; CHECK-NEXT:    addi r3, r3, .LCPI13_0@toc@l
-; CHECK-NEXT:    lxvd2x vs0, 0, r3
-; CHECK-NEXT:    xxswapd vs35, vs0
-; CHECK-NEXT:    xxlor vs34, vs34, vs35
+; CHECK-NEXT:    xvnabsdp vs34, vs34
 ; CHECK-NEXT:    blr
 entry:
   %conv = bitcast <2 x double> %a to <2 x i64>
   %and = or <2 x i64> %conv, <i64 -9223372036854775808, i64 -9223372036854775808>
   %conv1 = bitcast <2 x i64> %and to <2 x double>
   ret <2 x double> %conv1
 }
 

llvm/trunk/test/CodeGen/X86/fp-logic.ll

; CHECK-NEXT:    retq
   %bc2 = bitcast i32 %xor to float
   %fsub = fsub float %x, %bc2
   ret float %fsub
 }
 
 define float @nabsf(float %a) {
 ; CHECK-LABEL: nabsf:
 ; CHECK:       # %bb.0:
-; CHECK-NEXT:    movss {{.*#+}} xmm1 = mem[0],zero,zero,zero
-; CHECK-NEXT:    orps %xmm1, %xmm0
+; CHECK-NEXT:    orps {{.*}}(%rip), %xmm0
 ; CHECK-NEXT:    retq
   %conv = bitcast float %a to i32
   %and = or i32 %conv, -2147483648
   %conv1 = bitcast i32 %and to float
   ret float %conv1
 }
 
 define double @nabsd(double %a) {
 ; CHECK-LABEL: nabsd:
 ; CHECK:       # %bb.0:
-; CHECK-NEXT:    movsd {{.*#+}} xmm1 = mem[0],zero
-; CHECK-NEXT:    orps %xmm1, %xmm0
+; CHECK-NEXT:    orps {{.*}}(%rip), %xmm0
 ; CHECK-NEXT:    retq
   %conv = bitcast double %a to i64
   %and = or i64 %conv, -9223372036854775808
   %conv1 = bitcast i64 %and to double
   ret double %conv1
 }
 
 define <4 x float> @nabsv4f32(<4 x float> %a) {