This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] Follow-up to r322373 for materializing constants that set the CR
ClosedPublic

Authored by nemanjai on Jan 16 2018, 9:42 AM.

Details

Summary

Benjamin fixed an issue with the r+r -> r+i transform where we materialized an incorrect constant using ANDIo - thanks Ben.

However the fix is very conservative. It will only do the transform if AND-ing the input with the new constant produces the same new constant. This is of course correct, but not necessarily required.

  1. If the input constant isn't used by any other instructions, the immediate can be changed to the new constant. Even if the new constant would end up producing a negative value for the defining load-immediate, this isn't a problem because the only user will be the ANDIo
  2. If the GPR output from the instruction being replaced isn't used, it isn't relevant what constant is materialized in the GPR (as long as a zero is correctly identified). So the immediate operand of the ANDIo can just be zero or whatever the immediate operand is to the defining load-immediate.
  3. If neither of the above apply (or we don't know about uses due to being in the late peephole), we only transform if all bits of the new immediate are also set in the input constant (as Ben's fix currently ensures).

Examples:
Case 1:

li 3, 1
rlwinm. 4, 3, 2, 0, 31
# use CR0 and possibly GPR4 but not GPR3
### Transforms to:
li 3, 4
andi. 4, 3, 4

Case 2 (non-zero):

li 3, 1
rlwinm. 4, 3, 2, 0, 31
# Use CR0 and possibly GPR3, but not GPR4
li 3, 1
andi. 4, 3, 1

Case 2 (zero):

li 3, 1
rlwinm. 4, 3, 20, 21, 31
# Use CR0 and possibly GPR3, but not GPR4
li 3, 1
andi. 4, 3, 0

Diff Detail

Repository
rL LLVM

Event Timeline

nemanjai created this revision.Jan 16 2018, 9:42 AM

Ping. Does anyone on the reviewer list have some time to review this?

hfinkel added inline comments.May 29 2018, 11:55 AM
lib/Target/PowerPC/PPCInstrInfo.cpp
2501 ↗(On Diff #129970)

Indentation looks off here.

2516 ↗(On Diff #129970)

Could it be the case that NewImm is non-zero while Immediate is zero? Does it matter? (I'm guessing not, but given that NewImm is set in a variety of ways above, it's not completely obvious to me).

nemanjai marked 2 inline comments as done.May 31 2018, 12:36 AM
nemanjai added inline comments.
lib/Target/PowerPC/PPCInstrInfo.cpp
2516 ↗(On Diff #129970)

None of the instructions that could take the immediate from zero to non-zero have a record-form so it doesn't matter. However, to make it clear, I'll add an assert.

nemanjai updated this revision to Diff 149242.May 31 2018, 12:41 AM
nemanjai marked an inline comment as done.

Rebase the test cases with new sigils. Add an assert for an immediate that changes from zero to non-zero in transformation.

hfinkel accepted this revision.May 31 2018, 7:52 AM

LGTM

This revision is now accepted and ready to land.May 31 2018, 7:52 AM
This revision was automatically updated to reflect the committed changes.