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

[Sam Tebbs via Phabricator via llvm-commits]

samtebbs added inline comments.


================
Comment at: llvm/lib/Target/ARM/ARMISelDAGToDAG.cpp:3589
+      break;
+    auto Type = N->getValueType(0);
+    if (!Type.isVector())
----------------
dmgreen wrote:
> This is pretty chunky. I think it's worth moving to it's own function, like the other try* methods here.
Good idea. Does `tryFMUL` seem like a good name?


================
Comment at: llvm/lib/Target/ARM/ARMISelDAGToDAG.cpp:3604
+    // in 16 bit floats
+    if (ScalarBits == 16 && !FMulFlags.hasNoInfs())
+      break;
----------------
dmgreen wrote:
> I think can be "IsUnsigned" too. The values out of range are 65520+, so can only happen under unsigned convertions.
👍 


================
Comment at: llvm/lib/Target/ARM/ARMISelDAGToDAG.cpp:3607
+
+    if (RHS.getOpcode() == ISD::BITCAST)
+      RHS = RHS.getOperand(0);
----------------
dmgreen wrote:
> Bitcast can go wrong under BE if it's changing the type size. It can move the order of elements in the vector around. Is this only interested in cases where the element sizes are the same, and if so can it add a check?
It is indeed. I'll add a check to make sure that the element sizes are the same.


================
Comment at: llvm/lib/Target/ARM/ARMISelDAGToDAG.cpp:3618
+      case ARMISD::VDUP: {
+        unsigned Imm = RHS.getConstantOperandVal(0);
+        if (RHS.getOpcode() == ARMISD::VMOVIMM)
----------------
dmgreen wrote:
> If this is a vdup, it won't always have a constant operand.
👍 


================
Comment at: llvm/lib/Target/ARM/ARMISelDAGToDAG.cpp:3624
+                             : (ScalarBits == 16 ? APFloat::IEEEhalf()
+                                                 : APFloat::IEEEdouble()),
+                         APInt(ScalarBits, Imm));
----------------
dmgreen wrote:
> I think we ruled out 64bit floats above.
Indeed we did


================
Comment at: llvm/lib/Target/ARM/ARMISelDAGToDAG.cpp:3650
+                               &IsExact);
+      if (!IsExact || !Converted.isPowerOf2())
+        break;
----------------
dmgreen wrote:
> Should this be in some range too? Not negative? Do you have a few tests for cases like that?
I think you're right that there should be a range check, but I think it should come below where we compute FracBits as that must be less or equal to ScalarBits. Regarding it being negative, is that possible if it is a power of 2?


================
Comment at: llvm/lib/Target/ARM/ARMISelDAGToDAG.cpp:3659-3660
+
+      unsigned int Opcode = ARM::MVE_VCVTf32s32_fix;
+      switch (ScalarBits) {
+      case 16:
----------------
dmgreen wrote:
> Perhaps add a default: llvm_unreachable(..);  and then Opcode doesn't need a default value (but maybe 0 would be fine too?)
Good idea


================
Comment at: llvm/test/CodeGen/ARM/arm_q15_to_float_autovec.ll:2
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -O3 -mtriple=thumbv8.1m.main-arm-none-eabi %s -o - -mattr=+mve.fp | FileCheck %s
+
----------------
dmgreen wrote:
> Can you move the test to CodeGen/Thumb2/mve-<something>.ll, with all the others. Maybe mve-vcvt-fixed.ll? Also it shouldn't need -O3 on the llc line.
Ah yes! I totally forgot to rename the file.


================
Comment at: llvm/test/CodeGen/ARM/arm_q15_to_float_autovec.ll:4
+
+define dso_local <4 x float> @vcvt_i32_1(<4 x i32> %0) local_unnamed_addr {
+; CHECK-LABEL: vcvt_i32_1:
----------------
dmgreen wrote:
> Remove dso_local and the local_unnamed_addr  from all these tests, and add arm_aapcs_vfpcc. That should clean up a lot of the soft-float vmovs.
👍 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103903