[PATCH] D136916: [LoongArch] Fix codegen for [su]itofp instructions

[WÁNG Xuěruì via Phabricator via llvm-commits]

xen0n accepted this revision.
xen0n added inline comments.
This revision is now accepted and ready to land.


================
Comment at: llvm/lib/Target/LoongArch/LoongArchFloat32InstrInfo.td:290
+// uint -> f32
+def : Pat<(f32 (uint_to_fp (i64 (sexti32 (i64 GPR:$src))))),
+          (FFINT_S_W (MOVGR2FR_W GPR:$src))>;
----------------
wangleiat wrote:
> wangleiat wrote:
> > xen0n wrote:
> > > `sexti32` for `uint`? (I might be sleepy so this might not be wrong after all.) The codegen in the touched test cases look good though, this is a bit weird.
> > > `sexti32` for `uint`? (I might be sleepy so this might not be wrong after all.) The codegen in the touched test cases look good though, this is a bit weird.
> > 
> > `sexti32` means, can we treat the current node as a signed number.
> > > `sexti32` for `uint`? (I might be sleepy so this might not be wrong after all.) The codegen in the touched test cases look good though, this is a bit weird.
> > 
> > `sexti32` means, can we treat the current node as a signed number.
> 
> In fact, this matching rule only appears when `loongarch64 +f -d`, but I don't want to add the corresponding `hasNoBasicD()` feature, and the corresponding legality judgment has been made in the lowering stage. (In theory, this constraint can be removed.)
> > > `sexti32` for `uint`? (I might be sleepy so this might not be wrong after all.) The codegen in the touched test cases look good though, this is a bit weird.
> > 
> > `sexti32` means, can we treat the current node as a signed number.
> 
> In fact, this matching rule only appears when `loongarch64 +f -d`, but I don't want to add the corresponding `hasNoBasicD()` feature, and the corresponding legality judgment has been made in the lowering stage. (In theory, this constraint can be removed.)

Okay, so we can put off this for a while, knowing that you're well aware of the details (better than I am)... Feel free to come back to polish it later.


================
Comment at: llvm/lib/Target/LoongArch/LoongArchISelDAGToDAG.cpp:216-220
+  if (N.getOpcode() == LoongArchISD::BSTRPICK &&
+      N.getConstantOperandVal(1) < UINT64_C(0X1F) &&
+      N.getConstantOperandVal(2) == UINT64_C(0)) {
+    Val = N;
+    return true;
----------------
wangleiat wrote:
> xen0n wrote:
> > This makes me feel a bit nervous, because we conventionally use such `bstrpick` insns for zero extensions. The MSB immediate is tested to be strictly less than 32 so this is actually a guaranteed zero extension. Are we doing the right thing by matching the case in `sexti32` too?
> > This makes me feel a bit nervous, because we conventionally use such `bstrpick` insns for zero extensions. The MSB immediate is tested to be strictly less than 32 so this is actually a guaranteed zero extension. Are we doing the right thing by matching the case in `sexti32` too?
> 
> Yes, but when it only takes 31 bits, it can be considered either sign extension or zero extension.
Fair enough. Thanks for the explanation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136916