Page MenuHomePhabricator

[PPC] Move the combine "a << (b % (sizeof(a) * 8)) -> (PPCshl a, b)" to the backend. NFC.

Authored by timshen on May 10 2017, 3:23 PM.



Eli pointed out that it's unsafe to combine the shifts to ISD::SHL etc.,
because those are not defined for b > sizeof(a) * 8, even after some of
the combiners run.

However, PPCISD::SHL defines that behavior (as the instructions themselves).
Move the combination to the backend.

The tests in shift_mask.ll still pass.

Diff Detail


Event Timeline

timshen created this revision.May 10 2017, 3:23 PM
efriedma edited edge metadata.May 10 2017, 3:31 PM

Please update the comment in PPCISelLowering.h

13006 ↗(On Diff #98544)

Duplicated code; can you refactor these?

timshen updated this revision to Diff 98548.May 10 2017, 3:55 PM

Document PPCISD::SHL/SRA/SRL, and factor out common code.

timshen marked an inline comment as done.May 10 2017, 3:56 PM

Looks fine, but someone who knows PPC better should approve.

122 ↗(On Diff #98548)

Maybe leave the comment noting that 32-bit shifts are modulo 64?

timshen updated this revision to Diff 98564.May 10 2017, 5:05 PM

Add back the comment on scalar types to PPCISD::SHL/SRA/SRL. Verified the behavior by looking at Power ISA sld/srd/srad instructions.

timshen updated this revision to Diff 98566.May 10 2017, 5:10 PM

32-bit instructions like slw has a subtly different behavior. Make the documentation refer back to the ISA, since it's the intention.

timshen added inline comments.May 10 2017, 5:13 PM
122 ↗(On Diff #98548)


It's actually more subtle than "modulo 64". FWIW:

For (shl (i32 a), b):

if ((b % 64) >= 32) return 0;
return a << (b % 32)
timshen updated this revision to Diff 98655.May 11 2017, 10:30 AM

Update comments for accuracy.

iteratee added inline comments.May 11 2017, 11:21 AM
12957 ↗(On Diff #98655)

That's a typo.

126 ↗(On Diff #98655)

"For vector types, only the last n bits are used."

990 ↗(On Diff #98655)

Can these patterns go in a separate patch? They only seem partially related.

timshen added inline comments.May 11 2017, 11:26 AM
990 ↗(On Diff #98655)

They can, the problem is that there is no way to test that patch, since no one generates PPCshl on vector until this patch.

Do you think it'd be ok to have a separate patch being unable to test?

iteratee added inline comments.May 11 2017, 11:36 AM
990 ↗(On Diff #98655)

If you add the patterns, they should be generated. That was what happened when I added the vector shift patterns for v1i128

timshen added inline comments.May 11 2017, 12:40 PM
990 ↗(On Diff #98655)

The patterns will be generated, but they will match nothing at the time (therefore not easy to test).

This is because no one generate the SDNodes "PPCshl", aka PPCISD::SHL, for vector operations, without the changes in stripModuloOnShift().

iteratee added inline comments.May 11 2017, 1:11 PM
990 ↗(On Diff #98655)

OK, why do you need a separate node for shift?

timshen added inline comments.May 11 2017, 2:07 PM
990 ↗(On Diff #98655)
  1. It already exists for scalar types. I simply extend it for vector types as well.
  2. They are different because shl (ISD::SHL) has UB, but PPCshl (PPCISD::SHL) doesn't.

For example, (shl (i32 a), b) is UB when b >= 32. (PPCshl (i32 a), b), however, performs a << (b % 32) on vector types, as the instructions do too.

See discussion here:

iteratee added inline comments.May 11 2017, 2:42 PM
990 ↗(On Diff #98655)

OK, I get why you're doing it in dagcombiner, but why can't you just produce the instructions directly? Why do you need the intermediate nodes?

timshen added inline comments.May 11 2017, 3:02 PM
990 ↗(On Diff #98655)

Because I don't want to manually lower it to 8 * 2 = 16 instructions in C++ code. I prefer to write .td patterns for that.

Without PPCISD::SHL I either write C++, or write pattern to match ISD::SHL. But ISD::SHL doesn't have the semantic we want.

timshen updated this revision to Diff 98689.May 11 2017, 3:23 PM

Fix comments.

iteratee accepted this revision.May 11 2017, 3:54 PM

Looks fine with the comments I gave addressed.

1099 ↗(On Diff #98548)

Minor point, the patterns above are grouped by type and then by PPCxxx vs xxx
Can you group these the same?

990 ↗(On Diff #98655)

OK. I get the reasoning behind delaying the output.

This revision is now accepted and ready to land.May 11 2017, 3:54 PM
timshen added inline comments.May 11 2017, 4:06 PM
1099 ↗(On Diff #98548)

Actually this is consistent - the ordering is (operation, PPC or not, types).

This revision was automatically updated to reflect the committed changes.