[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 inline comments.


================
Comment at: llvm/test/CodeGen/LoongArch/ir-instruction/and.ll:82
+
+;; FIXME: remove redundant immediate style tests
+
----------------
SixWeining wrote:
> 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.
Thanks. This should make the diff much shorter ;-)


================
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
----------------
SixWeining wrote:
> 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?
Regarding splitting commits, I think either is fine, as the majority of changes here are actually test cases. You can of course do so if not in a hurry. ;-)


================
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
----------------
SixWeining wrote:
> 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.
Hmm, my reasoning is that if the incoming register `$a0` actually has non-zero bits set in the upper bits, the function signature is `i1 %a` for this parameter, so it would be UB if that's the case; hence doing anything is permissible, and we choose to ignore the bits (codegen-ing as if the non-0-or-1 inputs weren't possible) and just straight `maskeqz/masknez/or` away.

If this sounds okay, then we won't need the zero-extended marking after all.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127203