[AArch64][Optimization]Emit FCCMP for AND of two float compares

[ipriyanshi1708]

Summary

Transforms and(fcmp(a, b), fcmp(c, d)) into fccmp(fcmp(a, b), c, d)
Issue link: https://github.com/llvm/llvm-project/issues/60819

[ipriyanshi1708]

This comment has been deleted.

[ipriyanshi1708]

Generated the desired assembly

[ipriyanshi1708]

Update the tests

[ipriyanshi1708]

Updated the test file

[samtebbs]

Thank you for fixing up the other test changes, this is looking pretty good. I have some small changes to suggest.

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp

17158–17177

This doesn't quite match the LLVM style guide. You can try running git-clang-format HEAD^ to fix it automatically. If that doesn't work, you can re-style the code manually to make it look like the rest of the file.

17158–17177

I think this code would be nice to have in a function, perhaps named something like performANDSETCCCombine? Some comments about the transformation it performs (turning and(fcmp(a, b), fcmp(c, d)) into fccmp(fcmp(a, b), c, d)) would be good too.

[ipriyanshi1708]

Done the required changes

[samtebbs]

Thanks! Just a few more clean-up changes from me and I'll add another reviewer for a wider audience.

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp

17110

This could be moved to be just before the line it's used on.

17111

This could be moved to be just before where's it's used and AArch64CC seems like a verbose name so maybe just CC is enough.

17112

Same story with Cmp as it could be moved to be just before where it's set,

17128–17133

This can just return the value from DAG.getNode(...)

17135

We'll need a default return of SDValue() in case the if statements above don't pass.

[samtebbs]

llvm/test/CodeGen/AArch64/andcompare.ll

2525

Oh and a more explaining function name would be good, something like and_fcmp?

[ipriyanshi1708]

Updated the code

[samtebbs]

A couple more comments, then it looks good to me.

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp

17108

Some comments, about what transformation this function performs, would be useful.

17118

Perhaps rename this to CC? It's already clear that it's an AArch64 condition code.

[ipriyanshi1708]

Added the comments

[samtebbs]

Just one small change, but otherwise it looks good to me! I can commit this for you once you've moved the comment above the function, and then we can see about getting you commit access yourself.

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp

17133

I think this comment is quite useful and would be good to have on the line above the function definition.

[ipriyanshi1708]

Done the required changes

[ipriyanshi1708]

Fixed the failing test file

[samtebbs]

llvm/test/CodeGen/AArch64/select_fmf.ll

15–17 ↗

(On Diff #539834)

This change doesn't look beneficial because it adds extra instructions that aren't really needed. I think we'll need to find a way of restricting the code you've written to not transform this example. You can try looking at the debug output with the -print-after-all option added to llc to see what is different for this test case and the one you added. While testing you can copy this test file and remove everything but the two functions we're interested in to reduce the debug output when you run llc on it.

[hiraditya]

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp

17128

Have we already checked for and before calling emitConjunction ?

[ipriyanshi1708]

Fixed the code for the test file

[dmgreen]

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp

17142–17143

I think these may be better as DAG.getConstant(0, DL, VT), as opposed to AArch64::WZR / AArch64::XZR. They should get converted to the same thing eventually, but having zero constant's will be easier for the rest of DAG to reason about.

[samtebbs]

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp

17121

Perhaps add that LowerSELECT can be modified to work with the IR that this function produces, thereby removing the need for this check.

17128

Yep, this function is only called when an AND is found.

[ipriyanshi1708]

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp

17111

I kept it AArch64CC because CC is giving the error.

17121

Yep! We can update it in the future so that it works with the codegen.

17128

Sorry, I didn't get what you are trying to ask? Can you please elaborate little bit.

17142–17143

Yup! I tried that too but it was generating this instruction "cset w0, ne" instead of this one "cset w0, vs".

17158–17177

Okay! I will try to do it asap.

[dmgreen]

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp

17142–17143

Is that with something like this?

return DAG.getNode(AArch64ISD::CSINC, DL, VT,                      
                   DAG.getConstant(0, DL, VT),
                   DAG.getConstant(0, DL, VT),
                   DAG.getConstant(InvertedCC, DL, MVT::i32), Cmp);

[ipriyanshi1708]

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp

17142–17143

Ohh! yes it is correct. Earlier I have replaced DAG.getConstant(InvertedCC, DL, MVT::i32), Cmp); with DAG.getConstant(0, DL, VT) that's why it was not working.

[ipriyanshi1708]

Updated the code

[dmgreen]

From what I can see this LGTM. Thanks.

[samtebbs]

Thanks David. I can commit this for you if you'd like @ipriyanshi1708 . Shall I use the same email address and name as your previous commit?

[ipriyanshi1708]

In D152714#4569850, @samtebbs wrote:

Thanks David. I can commit this for you if you'd like @ipriyanshi1708 . Shall I use the same email address and name as your previous commit?

Yes! Please do commit it for me using the same email address and name.

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp

static SDValue performSVEAndCombine(SDNode *N,
   }
 
   if (isConstantSplatVectorMaskForType(Mask.getNode(), MemVT))
     return Src;
 
   return SDValue();
 }
 
+// Transform and(fcmp(a, b), fcmp(c, d)) into fccmp(fcmp(a, b), c, d)
+static SDValue performANDSETCCCombine(SDNode *N,
+                                      TargetLowering::DAGCombinerInfo &DCI) {
+
+  // This function performs an optimization on a specific pattern involving
+  // an AND operation and SETCC (Set Condition Code) node.
+
+  SDValue SetCC = N->getOperand(0);
+  EVT VT = N->getValueType(0);
+  SelectionDAG &DAG = DCI.DAG;
+
+  // Checks if the current node (N) is used by any SELECT instruction and
+  // returns an empty SDValue to avoid applying the optimization to prevent
+  // incorrect results
+  for (auto U : N->uses())
+    if (U->getOpcode() == ISD::SELECT)
+      return SDValue();
+
+  // Check if the operand is a SETCC node with floating-point comparison
+  if (SetCC.getOpcode() == ISD::SETCC &&
+      SetCC.getOperand(0).getValueType() == MVT::f32) {
+
+    SDValue Cmp;
+    AArch64CC::CondCode CC;
+
+    // Check if the DAG is after legalization and if we can emit the conjunction
+    if (!DCI.isBeforeLegalize() &&
+        (Cmp = emitConjunction(DAG, SDValue(N, 0), CC))) {
+
+      AArch64CC::CondCode InvertedCC = AArch64CC::getInvertedCondCode(CC);
+
+      SDLoc DL(N);
+      return DAG.getNode(AArch64ISD::CSINC, DL, VT, DAG.getConstant(0, DL, VT),
+                         DAG.getConstant(0, DL, VT),
+                         DAG.getConstant(InvertedCC, DL, MVT::i32), Cmp);
+    }
+  }
+  return SDValue();
+}
+
 static SDValue performANDCombine(SDNode *N,
                                  TargetLowering::DAGCombinerInfo &DCI) {
   SelectionDAG &DAG = DCI.DAG;
   SDValue LHS = N->getOperand(0);
   SDValue RHS = N->getOperand(1);
   EVT VT = N->getValueType(0);
 
   if (SDValue R = performANDORCSELCombine(N, DAG))
     return R;
 
+  if (SDValue R = performANDSETCCCombine(N,DCI))
+    return R;
+
   if (!DAG.getTargetLoweringInfo().isTypeLegal(VT))
     return SDValue();
 
   if (VT.isScalableVector())
     return performSVEAndCombine(N, DCI);
 
   // The combining code below works only for NEON vectors. In particular, it
   // does not work for SVE when dealing with vectors wider than 128 bits.
   if (!VT.is64BitVector() && !VT.is128BitVector())
     return SDValue();
 
   BuildVectorSDNode *BVN = dyn_cast<BuildVectorSDNode>(RHS.getNode());
   if (!BVN)
     return SDValue();
 
   // AND does not accept an immediate, so check if we can use a BIC immediate
   // instruction instead. We do this here instead of using a (and x, (mvni imm))
   // pattern in isel, because some immediates may be lowered to the preferred
   // (and x, (movi imm)) form, even though an mvni representation also exists.
   APInt DefBits(VT.getSizeInBits(), 0);
   APInt UndefBits(VT.getSizeInBits(), 0);
   if (resolveBuildVector(BVN, DefBits, UndefBits)) {
     SDValue NewOp;
 
     // Any bits known to already be 0 need not be cleared again, which can help

llvm/test/CodeGen/AArch64/andcompare.ll

 ; GISEL-NEXT:    csel w0, w8, wzr, hs
 ; GISEL-NEXT:    ret
   %and = and i32 %num, 254
   %cmp = icmp uge i32 %and, 16
   %r = select i1 %cmp, i32 %and, i32 0
   ret i32 %r
 }
 
+define i1 @and_fcmp(float %0, float %1) {
+; SDISEL-LABEL: and_fcmp:
+; SDISEL:       // %bb.0:
+; SDISEL-NEXT:    fcmp s1, s1
+; SDISEL-NEXT:    fccmp s0, s0, #0, vs
+; SDISEL-NEXT:    cset w0, vs
+; SDISEL-NEXT:    ret
+;
+; GISEL-LABEL: and_fcmp:
+; GISEL:       // %bb.0:
+; GISEL-NEXT:    fcmp s0, #0.0
+; GISEL-NEXT:    cset w8, vs
+; GISEL-NEXT:    fcmp s1, #0.0
+; GISEL-NEXT:    cset w9, vs
+; GISEL-NEXT:    and w0, w8, w9
+; GISEL-NEXT:    ret
+
+  %3 = fcmp uno float %0, 0.000000e+00
+  %4 = fcmp uno float %1, 0.000000e+00
+  %5 = and i1 %3, %4
+  ret i1 %5
+}
+
 
 ;; NOTE: These prefixes are unused and the list is autogenerated. Do not add tests below this line:
 ; CHECK: {{.*}}