[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:3192
+    // that don't
+    if (!dyn_cast<ConstantSDNode>(RHS.getOperand(0)))
+      return false;
----------------
dmgreen wrote:
> samtebbs wrote:
> > dmgreen wrote:
> > > Can use isa, as opposed to dyn_cast.
> > > 
> > > I'm not sure what the TODO is related to?
> > Good idea 👍 
> > 
> > The TODO is saying that when we find examples of VDUPs that don't have a constant operand we should modify this to work with them.
> I think we are only interested in constants. We have tests for all the cases, so I'm not sure there will be cases without the operand to the vdup being a constant. And if they weren't constants, it sounds difficult to try and do anything with them here.
> 
> i.e, I think you can remove the TODO :)
Agreed :)


================
Comment at: llvm/test/CodeGen/Thumb2/mve-vcvt-fixed.ll:771
+; CHECK:       @ %bb.0:
+; CHECK-NEXT:    vcvt.f32.u32 q0, q0, #31
+; CHECK-NEXT:    bx lr
----------------
dmgreen wrote:
> This one shouldn't be converted, if the 0xBE00000000000000 is negative.
> 
> With a constant of 0x3E00000000000000 (which is really 0x30000000 as a float not a double, which is apparently 4.65661287308e-10, which is 1.0f/0x80000000) it should be converted, as far as I understand.
Ah yes. When checking if the immediate's top bit is set I also check if the vector is signed, but of course floats are always signed so I'll need to remove the `!IsUnsigned` check. Thanks!


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

https://reviews.llvm.org/D103903