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

[Dave Green via Phabricator via llvm-commits]

dmgreen added a comment.

Nice one. Sounds funky.



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


================
Comment at: llvm/lib/Target/ARM/ARMISelDAGToDAG.cpp:3604
+    // in 16 bit floats
+    if (ScalarBits == 16 && !FMulFlags.hasNoInfs())
+      break;
----------------
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);
----------------
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?


================
Comment at: llvm/lib/Target/ARM/ARMISelDAGToDAG.cpp:3614
+      APFloat ImmAPF(0.0f);
+      bool valid = true;
+      switch (RHS.getOpcode()) {
----------------
"valid" would be "Valid", but this may not be needed if this was a separate function. It can just return false out of the default then.


================
Comment at: llvm/lib/Target/ARM/ARMISelDAGToDAG.cpp:3618
+      case ARMISD::VDUP: {
+        unsigned Imm = RHS.getConstantOperandVal(0);
+        if (RHS.getOpcode() == ARMISD::VMOVIMM)
----------------
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));
----------------
I think we ruled out 64bit floats above.


================
Comment at: llvm/lib/Target/ARM/ARMISelDAGToDAG.cpp:3650
+                               &IsExact);
+      if (!IsExact || !Converted.isPowerOf2())
+        break;
----------------
Should this be in some range too? Not negative? Do you have a few tests for cases like that?


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


================
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
+
----------------
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.


================
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:
----------------
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.


================
Comment at: llvm/test/CodeGen/ARM/arm_q15_to_float_autovec.ll:434
+  %2 = sitofp <4 x i32> %0 to <4 x float>
+  %3 = fmul <4 x float> %2, <float 0xBE00000000000000, float 0xBE00000000000000, float 0xBE00000000000000, float 0xBE00000000000000>
+  ret <4 x float> %3
----------------
This kind of looks negative ;)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103903