[PATCH] D127203: [LoongArch] Add codegen support for the bitwise binary operations and part of other operations

[Lu Weining via Phabricator via llvm-commits]

SixWeining marked an inline comment as done.
SixWeining added inline comments.


================
Comment at: llvm/lib/Target/LoongArch/LoongArchFloat32InstrInfo.td:153
+
+//TODO: Match signaling comparison `strict_fsetccs` with FCMP_S*_S instructions.
+
----------------
xen0n wrote:
> nit: one space before `TODO`
OK. Thanks.


================
Comment at: llvm/lib/Target/LoongArch/LoongArchISelLowering.cpp:226
+  default:
+    llvm_unreachable("Don't know how to custom type legalize this operation!");
+  case ISD::SHL:
----------------
xen0n wrote:
> nit: Chinglish; maybe just "Don't know how to legalize this operation" would suffice.
OK, thanks.


================
Comment at: llvm/test/CodeGen/LoongArch/ir-instruction/and.ll:82
+
+;; FIXME: remove redundant immediate style tests
+
----------------
xen0n wrote:
> I suppose you mean removing the redundant test cases with known out-of-bounds operands here; do you plan to just remove them before landing?
Yes. Let me remove some redundant tests. Thanks.


================
Comment at: llvm/test/CodeGen/LoongArch/ir-instruction/select-bare-flt.ll:10-11
+; LA32:       # %bb.0:
+; LA32-NEXT:    andi $a0, $a0, 1
+; LA32-NEXT:    movgr2cf $fcc0, $a0
+; LA32-NEXT:    fsel $fa0, $fa1, $fa0, $fcc0
----------------
xen0n wrote:
> `movgr2cf` only looks at the LSB, so the `andi` is unnecessary and we could save a cycle here. However this may well be because of some codegen details (I think it's due to codegen wanting to first extend `i1` to ensure the upper bits have well-defined content), and this can be improved later. This patch is too large after all...
Good idea! Let me record and improve it in future patch.

Sorry. Yes, this patch is big. How about let me try to split it into 3~5 small ones firstly?


================
Comment at: llvm/test/CodeGen/LoongArch/ir-instruction/select-bare-int.ll:9
+; LA32:       # %bb.0:
+; LA32-NEXT:    andi $a0, $a0, 1
+; LA32-NEXT:    masknez $a2, $a2, $a0
----------------
xen0n wrote:
> Similarly here; this `andi` is unnecessary too in this case. (`$a0` is the `i1 %a`, so I think it should be UB if upper bits contain interesting content; I'm not quite sure about this though. If it's indeed UB then the suggestion is appropriate.)
I think the `andi 1` is necessary here because `maskeqz` and `masknez` will look at if `$a0` is `0` or not. So we must make sure the upper bits are all zeros.

However, if we indicate the codegen that the parameter `i1 %a` is [[ https://llvm.org/docs/LangRef.html#parameter-attributes | zero-extended  ]]before passed in, the `andi 1` would be omitted.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127203