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

[Priyanshi Agarwal via Phabricator via llvm-commits]

ipriyanshi1708 added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:16397
+  // returns an empty SDValue to avoid applying the optimization to prevent
+  // incorrect results
+  for (auto U : N->uses())
----------------
samtebbs wrote:
> Perhaps add that LowerSELECT can be modified to work with the IR that this function produces, thereby removing the need for this check.
Yep! We can update it in the future so that it works with the codegen.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:16418-16419
+      return DAG.getNode(AArch64ISD::CSINC, DL, VT,
+                         DAG.getRegister(ZeroReg, VT),
+                         DAG.getRegister(ZeroReg, VT),
+                         DAG.getConstant(InvertedCC, DL, MVT::i32), Cmp);
----------------
dmgreen wrote:
> 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.
Yup! I tried that too but it was generating this instruction "cset	w0, ne" instead of this one "cset    w0, vs".


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:16404
+    if (!DCI.isBeforeLegalize() &&
+        (Cmp = emitConjunction(DAG, SDValue(N, 0), CC))) {
+
----------------
hiraditya wrote:
> Have we already checked for `and` before calling emitConjunction ?
Sorry, I didn't get what you are trying to ask? Can you please elaborate little bit.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:16387
+  SDLoc DL(N);
+  AArch64CC::CondCode AArch64CC;
+  SDValue Cmp, Cset;
----------------
samtebbs wrote:
> 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.
I kept it AArch64CC because CC is giving the error.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:16394-16413
+  SDLoc DL(N);
+  AArch64CC::CondCode AArch64CC;
+  SDValue Cmp,Cset;
+  SDValue SetCC = N->getOperand(0);
+if (SetCC.getOpcode() == ISD::SETCC && SetCC.getOperand(0).getValueType() == MVT::f32){
+
+  if (!DCI.isBeforeLegalize() &&
----------------
samtebbs wrote:
> samtebbs wrote:
> > This doesn't quite match the [[ https://llvm.org/docs/CodingStandards.html#source-code-formatting | 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.
> 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.
Okay! I will try to do it asap.



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152714/new/

https://reviews.llvm.org/D152714