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

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

xen0n added a comment.

The implementation and test cases LGTM in terms of LoongArch semantics, only some minor nits.

Other people may have to check for general LLVM coding practices and style though.



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


================
Comment at: llvm/lib/Target/LoongArch/LoongArchFloat64InstrInfo.td:168
+
+//TODO: Match signaling comparison `strict_fsetccs` with FCMP_S*_D instructions.
+
----------------
nit: ditto


================
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:
----------------
nit: Chinglish; maybe just "Don't know how to legalize this operation" would suffice.


================
Comment at: llvm/test/CodeGen/LoongArch/ir-instruction/and.ll:82
+
+;; FIXME: remove redundant immediate style tests
+
----------------
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?


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


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127203