[PATCH] D156538: [AArch64] Try to combine FMUL with FDIV

[Dave Green via Phabricator via llvm-commits]

dmgreen added a comment.

In D156538#4579511 <https://reviews.llvm.org/D156538#4579511>, @jaykang10 wrote:

> @dmgreen As you can see, there are some regressions.
> After exact inverse for `fmul`, the `convertToInteger` sets `IsExact` to `false` in some cases.
> Maybe, we could need to keep the `fdiv` patterns too...

I think that any test with a fdiv, that would by instcombine be converted to a fmul (like https://godbolt.org/z/P1bd73TK7) are OK to leave as regressions. We would not see the regression in practice as instcombine has already performed the conversion to a fmul.

Do you know which cases that would not cover and would be left over if the fdiv was not handled?



================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:16717
+/// floating-point conversion.
+static SDValue performFDivCombine(SDNode *N, SelectionDAG &DAG,
+                                  TargetLowering::DAGCombinerInfo &DCI,
----------------
I think this can be dropped. If we want this transform for scalars it would be best to reuse the logic in DAGCombine.


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrFormats.td:709-711
+  let EncoderMethod = "getFixedPointScaleOpValue";
+  let DecoderMethod = "DecodeFixedPointScaleImm32";
+  let ParserMatchClass = Imm1_32Operand;
----------------
This can maybe be removed, as the class is only used as a ComplexPattern, not as a assembly Operand?


================
Comment at: llvm/test/CodeGen/AArch64/svtcf-fmul-fdiv-combine.ll:4
+
+define float @scvtf_f32_2(i32 %state) {
+; CHECK-LABEL: scvtf_f32_2:
----------------
Can you add some fp16 variants in here too.


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

https://reviews.llvm.org/D156538