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

[wanglei via Phabricator via llvm-commits]

wangleiat added a comment.

In D136916#3893657 <https://reviews.llvm.org/D136916#3893657>, @xen0n wrote:

> Fixed formatting of commit message for you.
>
> Note that there seems to be widespread misuse of `[]` and `{}` shorthand notations among Loongson people: they come from //shell syntax//, and should look like `[abc]`, `[a-c]`, `{a,b}` or `{,a}`, //never// `[a/b]` or `{a/b}`. Please keep this in mind (even the LoongArch ISA manual got this wrong, IMO it's better to tell someone to amend the manual instead of allowing the current practice last indefinitely).

Thanks!



================
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))>;
----------------
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.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136916