This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by NeHuang on Oct 21 2021, 5:49 PM.

Details

Summary

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.

Diff Detail

Event Timeline

NeHuang created this revision.Oct 21 2021, 5:49 PM
NeHuang requested review of this revision.Oct 21 2021, 5:49 PM
nemanjai added inline comments.Oct 26 2021, 4:18 AM
llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp
5003

I think a couple of improvements can be made here:

  1. Populate the Opcode and Ops array based on various conditions and add a single call to SelectNodeTo()
  2. Handle the first input being constant (and then flip the condition)
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.

nemanjai requested changes to this revision.Oct 26 2021, 4:55 AM
This revision now requires changes to proceed.Oct 26 2021, 4:55 AM
amyk added inline comments.Oct 26 2021, 7:44 AM
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.

NeHuang updated this revision to Diff 383506.Oct 29 2021, 2:49 PM

Addressed review comments from @nemanjai and @amyk

NeHuang marked 3 inline comments as done.Oct 29 2021, 2:50 PM
amyk added inline comments.Nov 1 2021, 3:35 PM
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.

NeHuang added inline comments.Nov 3 2021, 7:54 AM
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.

nemanjai requested changes to this revision.Nov 3 2021, 5:25 PM
nemanjai added inline comments.
llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp
5001

+1

5003

+1
Same thing with the Ops vector. Pre-populate it and then only change the operand that needs to be changed.

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

This revision now requires changes to proceed.Nov 3 2021, 5:25 PM
NeHuang updated this revision to Diff 384479.Nov 4 2021, 5:17 AM
NeHuang marked 5 inline comments as done.

Addressed review comments from @amy

NeHuang marked 2 inline comments as done.Nov 4 2021, 5:25 AM
NeHuang updated this revision to Diff 384768.Nov 4 2021, 2:18 PM
NeHuang marked 3 inline comments as done.

Address review comments from @nemanjai

nemanjai accepted this revision.Nov 5 2021, 4:13 AM

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

This revision is now accepted and ready to land.Nov 5 2021, 4:13 AM
amyk accepted this revision.Nov 5 2021, 8:30 AM

Aside from Nemanja's comments, this patch LGTM. Thanks for addressing the comments!