[PATCH] D104793: [ARM] Transform a floating-point to fixed-point conversion to a VCVT_fix

[Dave Green via Phabricator via llvm-commits]

dmgreen added a comment.

Nice. I wasn't expecting this to share as much code, that's good to see.



================
Comment at: llvm/lib/Target/ARM/ARMISelDAGToDAG.cpp:3157
 
-bool ARMDAGToDAGISel::tryFMULFixed(SDNode *N, SDLoc dl) {
-  // Transform a fixed-point to floating-point conversion to a VCVT
-  if (!Subtarget->hasMVEFloatOps())
-    return false;
+bool ARMDAGToDAGISel::transformFixedFloatingPointConversion(SDNode *N, SDLoc dl,
+                                                            SDNode *FMul,
----------------
You can likely not pass dl, using SDLoc(N) again here.


================
Comment at: llvm/lib/Target/ARM/ARMISelDAGToDAG.cpp:3173-3174
 
+  auto ImmNode = FMul->getOperand(1);
+  auto VecVal = FMul->getOperand(0);
+
----------------
auto -> SDValue

Should VecVal be different for the two cases? Otherwise below it uses the fmul->operand(0)->operand(0).


================
Comment at: llvm/lib/Target/ARM/ARMISelDAGToDAG.cpp:3176
+
+  if (VecVal.getValueType().getVectorElementType().getSizeInBits() !=
+      ScalarBits)
----------------
Can this use getScalarSizeInBits?


================
Comment at: llvm/lib/Target/ARM/ARMISelDAGToDAG.cpp:3204
 
-  // Multiplying by a factor of 2^(-n) will convert from fixed point to
-  // floating point, where n is the number of fractional bits in the fixed
-  // point number. Taking the inverse and log2 of the factor will give n
-  APFloat Inverse(0.0f);
-  if (!ImmAPF.getExactInverse(&Inverse))
-    return false;
-
+  // Multiplying by a factor of 2^n will convert from floating point to
+  // fixed point, where n is the number of fractional bits in the fixed
----------------
Maybe expand the comment to talk about both FixedToFloat and FloatToFixed


================
Comment at: llvm/lib/Target/ARM/ARMISelDAGToDAG.cpp:3254
+    return false;
+  auto Type = N->getValueType(0);
+  if (!Type.isVector())
----------------
auto -> EVT


================
Comment at: llvm/lib/Target/ARM/ARMISelDAGToDAG.cpp:3260
+  bool IsUnsigned = N->getOpcode() == ISD::FP_TO_UINT;
+  auto Node = N->getOperand(0).getNode();
+
----------------
auto -> SDNode *


================
Comment at: llvm/lib/Target/ARM/ARMISelDAGToDAG.cpp:3310
+  return transformFixedFloatingPointConversion(
+      N, dl, N, LHS.getOpcode() == ISD::UINT_TO_FP, true);
+}
----------------
The second N should be LHS?


================
Comment at: llvm/test/CodeGen/Thumb2/mve-vcvt-float-to-fixed.ll:802
+
+define arm_aapcs_vfpcc <4 x float> @vcvt_u32_32(<4 x i32> %0) {
+; CHECK-LABEL: vcvt_u32_32:
----------------
Does this need updating? and vcvt_u32_33 below.


================
Comment at: llvm/test/CodeGen/Thumb2/mve-vcvt-float-to-fixed.ll:986
+
+define arm_aapcs_vfpcc <8 x i16> @vcvt_s16_inf(<8 x half> %0) {
+; CHECK-LABEL: vcvt_s16_inf:
----------------
These too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104793