This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] avoid masking already-zero bits in BitPermutationSelector
ClosedPublic

Authored by inouehrs on Jun 11 2018, 7:11 AM.

Details

Summary

The current BitPermutationSelector generates a code to build a value by tracking two types of bits: ConstZero and Variable.
ConstZero means a bit we need to mask off and Variable is a bit we copy from an input value.

This patch add third type of bits VariableKnownToBeZero caused by AssertZext node or zero-extending load node.
VariableKnownToBeZero is also a bit comes from an input value, but it is known to be already zero. So we do not need to mask them.
VariableKnownToBeZero enhances flexibility to group bits, since we can avoid redundant masking for these bits.

This patch also renames "HasZero" to "NeedMask" since now we may skip masking even when we have zeros (of type VariableKnownToBeZero).

Diff Detail

Repository
rL LLVM

Event Timeline

inouehrs created this revision.Jun 11 2018, 7:11 AM

Seems like a good idea. A couple of questions below.

lib/Target/PowerPC/PPCISelDAGToDAG.cpp
1319 ↗(On Diff #150742)

Why was this changed? I'm fine with it either way, but I think that we should be consistent. Either change the type for V and V.getOperand(0), or the number of bits for both.

1426 ↗(On Diff #150742)

Couldn't you end up here with a single group that's a mixture of constant zeros and known-to-be-zero bits from different underlying values (with, potentially, different rotation factors)? If so, isn't that a problem?

inouehrs updated this revision to Diff 151708.Jun 18 2018, 7:03 AM
inouehrs marked an inline comment as done.Jun 18 2018, 7:09 AM
inouehrs added inline comments.
lib/Target/PowerPC/PPCISelDAGToDAG.cpp
1319 ↗(On Diff #150742)

I forgot to remove irrelevant test code unintentionally. Thank you for pointing this out.

1426 ↗(On Diff #150742)

Known-to-be-zero bits from multiple underlying values can be in the same group of zeros. It is ok if they are known to be zero. Masking bits and copying these (zero) bits from the underlying values should give the same result.
What we need to really care is not to include non-zero bits in a group of zeros, which is checked in line 1434 below.

inouehrs marked an inline comment as done.Jul 2 2018, 6:12 AM

gentle ping

inouehrs updated this revision to Diff 154592.Jul 9 2018, 6:41 AM
  • rebase to the latest
  • make the test cases more strict

gentle ping

inouehrs updated this revision to Diff 162881.Aug 28 2018, 8:42 AM

rebased to the latest revision

@hfinkel do you have any further suggestions?

inouehrs updated this revision to Diff 169004.Oct 10 2018, 7:05 AM

gentle ping

gentle ping

Ack. Sorry. I'll look this over again today.

hfinkel accepted this revision.Oct 11 2018, 7:48 PM

LGTM

This revision is now accepted and ready to land.Oct 11 2018, 7:48 PM
This revision was automatically updated to reflect the committed changes.