This patch adds the backend optimization to match XL behavior for the two builtins __tdw and __tw that when the second input argument is an immediate, emitting tdi/twi instructions instead of td/tw.
Details
- Reviewers
nemanjai stefanp amyk - Group Reviewers
Restricted Project - Commits
- rG18fe0a0d9eb1: [PowerPC] PPC backend optimization to lower int_ppc_tdw/int_ppc_tw intrinsics…
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp | ||
---|---|---|
5003 | I think a couple of improvements can be made here:
| |
llvm/test/CodeGen/PowerPC/builtins-ppc-xlcompat-trap-64bit-only.ll | ||
131 | 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. |
llvm/test/CodeGen/PowerPC/builtins-ppc-xlcompat-trap-64bit-only.ll | ||
---|---|---|
131 | 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. |
llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp | ||
---|---|---|
5001 | Might be a good idea to save N->getConstantOperandVal(1) since it is being accessed quite a few times here. | |
5003 | 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? | |
5038 | nit: Curly braces can be removed. | |
5042 | nit: Curly braces can be removed. | |
llvm/test/CodeGen/PowerPC/builtins-ppc-xlcompat-trap-64bit-only.ll | ||
131 | 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. |
llvm/test/CodeGen/PowerPC/builtins-ppc-xlcompat-trap-64bit-only.ll | ||
---|---|---|
131 | Good catch. Let me rebase this patch with ToT. The NFC patch was committed at 40cad47fd82ecaf253ba9b11fcd34f67dd557e9d. |
llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp | ||
---|---|---|
5001 | +1 | |
5003 | +1 | |
5012 | 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. | |
5036 | This will be an uninitialized variable when compiled without asserts. | |
llvm/test/CodeGen/PowerPC/builtins-ppc-xlcompat-trap-64bit-only.ll | ||
3–7 | This change should just be pre-committed |
LGTM other than a number of stylistic changes. Feel free to address those on the commit. You also might want to give @amyk a bit of time to ensure her comments were adequately addressed.
llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp | ||
---|---|---|
5017 | Nit: no braces for a single statement. | |
5020 | Nit: keep the else on the line immediately following the end of the if block so it is visually easy to match them up (the comments for else/else if go into the block). Also, rather than the structure: if (something) { } else { if (something else) ... else ... } Opt for a more flat structure of if (something) else if (something else) else | |
5021 | If you initialize Opcode this way at the declaration, you won't need this here and can flatten this as per my above comment. | |
5025 | Nit: no braces with a single statement. | |
5037 | s/fifthy/fifth |
Might be a good idea to save N->getConstantOperandVal(1) since it is being accessed quite a few times here.