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

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai requested changes to this revision.
nemanjai added inline comments.
This revision now requires changes to proceed.


================
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) {
----------------
amyk wrote:
> Might be a good idea to save `N->getConstantOperandVal(1)` since it is being accessed quite a few times here.
+1


================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:5003
+        N->getConstantOperandVal(1) == Intrinsic::ppc_tw) {
+      unsigned Opcode = 0;
+      int16_t SImmOperand2;
----------------
amyk wrote:
> 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?
+1
Same thing with the `Ops` vector. Pre-populate it and then only change the operand that needs to be changed.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:5012
+          isIntS16Immediate(N->getOperand(3), SImmOperand3);
+      // Will emit TD/TW if 2nd and 3rd operands are reg + reg or imm + imm
+      if (isOperand2IntS16Immediate == isOperand3IntS16Immediate) {
----------------
Nit: complete sentences please. Here and in other comments.

Also, please add a comment stating that the `imm + imm` form will be optimized to either an unconditional trap or a nop in a later pass.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:5036
+               "4th operand is not an Immediate");
+        int16_t TO = int(SImmOperand4) & 0x1F;
+        // when first and second bit of TO not same, swap them
----------------
This will be an uninitialized variable when compiled without asserts.


================
Comment at: llvm/test/CodeGen/PowerPC/builtins-ppc-xlcompat-trap-64bit-only.ll:3-7
+; RUN:   --ppc-asm-full-reg-names -mcpu=pwr8 < %s | FileCheck %s
 ; RUN: llc -verify-machineinstrs -mtriple=powerpc64-unknown-linux-gnu \
-; RUN:   -mcpu=pwr7 < %s | FileCheck %s
+; RUN:   --ppc-asm-full-reg-names -mcpu=pwr7 < %s | FileCheck %s
 ; RUN: llc -verify-machineinstrs -mtriple=powerpc64-unknown-aix \
+; RUN:   --ppc-asm-full-reg-names -mcpu=pwr8 < %s | FileCheck %s
----------------
This change should just be pre-committed


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112285