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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
