[PATCH] D112285: [PowerPC] PPC backend optimization to lower int_ppc_tdw/int_ppc_tw intrinsics to TDI/TWI machine instructions

[Amy Kwan via Phabricator via cfe-commits]

amyk added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:5001
+  case ISD::INTRINSIC_VOID: {
+    if (N->getConstantOperandVal(1) == Intrinsic::ppc_tdw ||
+        N->getConstantOperandVal(1) == Intrinsic::ppc_tw) {
----------------
Might be a good idea to save `N->getConstantOperandVal(1)` since it is being accessed quite a few times here.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:5003
+        N->getConstantOperandVal(1) == Intrinsic::ppc_tw) {
+      unsigned Opcode = 0;
+      int16_t SImmOperand2;
----------------
I see we emit TDI/TWI in 2/3 cases, so I was wondering if it make sense pull out setting the opcode in the second and third case to have the default opcode be:
```
        Opcode = N->getConstantOperandVal(1) == Intrinsic::ppc_tdw ? PPC::TDI
                                                                   : PPC::TWI;
```
And then we just set the opcode to TD/TW in the first case?


================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:5038
+        // when first and second bit of TO not same, swap them
+        if ((TO & 0x1) != ((TO & 0x2) >> 1)) {
+          TO = (TO & 0x1) ? TO + 1 : TO - 1;
----------------
nit: Curly braces can be removed.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:5042
+        // when third and fourth bit of TO not same, swap them
+        if ((TO & 0x8) != ((TO & 0x10) >> 1)) {
+          TO = (TO & 0x8) ? TO + 8 : TO - 8;
----------------
nit: Curly braces can be removed.


================
Comment at: llvm/test/CodeGen/PowerPC/builtins-ppc-xlcompat-trap-64bit-only.ll:131
+; CHECK:       # %bb.0:
+; CHECK-NEXT:    tdi 3, 3, 32767
+; CHECK-NEXT:    blr
----------------
amyk wrote:
> nemanjai wrote:
> > Can we add `-ppc-asm-full-reg-names` to the RUN lines so it is more clear which operand is a register and which is an immediate. This works on AIX now since https://reviews.llvm.org/D94282 landed.
> Maybe it would be good to pre-commit the change with `-ppc-asm-full-reg-names` added to the run lines so then this patch can only contain the pertinent `td`/`tdi`/`tw`/`twi` changes.
I meant, maybe it is a better idea to commit the test cases with `-ppc-asm-full-reg-names` first, so then this revision does not contain the additional updates of adding the registers in places that is not affected by your patch. However, perhaps if Nemanja thinks adding the option to this patch is OK, then that's fine with me, too. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112285