Page MenuHomePhabricator

[PowerPC] Exploit the rlwinm instructions for "and" with constant.
ClosedPublic

Authored by steven.zhang on Dec 23 2019, 12:58 AM.

Details

Summary

For now, PowerPC will using several instructions to get the constant and "and" it with the following case:

define i32 @test1(i32 %a) {
  %and = and i32 %a, -2
  ret i32 %and

However, we could exploit it with the rotate mask instructions.

               MB  ME
+----------------------+
|xxxxxxxxxxx00011111000|
+----------------------+
 0         32         64

Notice that, we can only do it if the MB is larger than 32 and MB <= ME as RLWINM will replace the content of [0 - 32) with [32 - 64) even we didn't rotate it.

Diff Detail

Event Timeline

steven.zhang created this revision.Dec 23 2019, 12:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 23 2019, 12:58 AM
shchenz added inline comments.Dec 25 2019, 7:56 PM
llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp
4467

Seems if you only consider MB>=32 && MB<=ME case, i think current isRunOfOnes for type i32 should match your requirement? Before call isRunOfOnes() check whether Imm64 can be represented by i32?

4468

for and reg, 0xffffffffffff00ffULL, it should be replaced by one single rlwinm instruction, seems it is not handled here?

steven.zhang marked an inline comment as done.Dec 25 2019, 8:58 PM
steven.zhang added inline comments.
llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp
4468

Good point. I will add it together.

steven.zhang marked an inline comment as done.Dec 25 2019, 9:44 PM
steven.zhang added inline comments.
llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp
4468

Hmm, after double check, we cannot do it with single rlwinm instructions as it is a wrapping, and the content [0, 31] has been filled with the content of [32, 63]. We need another instructions to clear it. That is the case of 'else' clause.

steven.zhang marked an inline comment as done.Dec 25 2019, 9:46 PM
steven.zhang added inline comments.
llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp
4467

We need the isRunOfOnes64 as we will do the special handling for all the cases that is RunOfOne64 with two instructions.

shchenz added inline comments.Dec 25 2019, 10:27 PM
llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp
4468

Sorry, I made a mistake here. and reg, 0xffffffffffff00ffULL I think this case is not a good candidate for one single rlwinm instruction. We get a wrap mask and the result is only related to the low 32 bit of and input. But from the mask 0xffffffffffff00ffULL, the result should be also related to high 32 bit of and input. But I think it is a good candidate for your patch https://reviews.llvm.org/D71831.

llvm/test/CodeGen/PowerPC/opt-cmp-inst-cr0-live.ll
10

The case change seems strange here, the left uses a record form ANDI8, so we can eliminate the CMPLDI with 0 after peephole. But with this change, ANDI8 is converted to RLWINM8, which is not record form. but it surprises me we still can delete the CMPLDI with 0 and replace RLWINM8 back with ANDI8. I think here we should use record form RLWINM8o. But I think it is not this patch's scope.

steven.zhang marked an inline comment as done.Dec 25 2019, 11:24 PM
steven.zhang added inline comments.
llvm/test/CodeGen/PowerPC/opt-cmp-inst-cr0-live.ll
10

If it is NOT 16 bit, it is transformed to RLWINM8 record form. And it makes sense to me.

define signext i32 @fn1(i32 %baz) {
  %1 = mul nsw i32 %baz, 208
  %2 = zext i32 %1 to i64
  %3 = shl i64 %2, 47
  %4 = ashr exact i64 %3, 47
  %5 = icmp eq i64 %4, 0
  br i1 %5, label %foo, label %bar
foo:
  ret i32 1

bar:
  ret i32 0
}
	mulli 3, 3, 208
	rlwinm. 3, 3, 0, 15, 27
	beq	0, .LBB0_2

Remove the 16-bit limit as we will have benefit even it is within 16bit, as rlwinm won't define the CR register while ANDIo did.

shchenz accepted this revision.Dec 26 2019, 1:52 AM

LGTM with one nit comment.

llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp
4460

imm16 should be handle as well in new patch? comment should update?

This revision is now accepted and ready to land.Dec 26 2019, 1:52 AM
steven.zhang marked an inline comment as done.
steven.zhang added inline comments.
llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp
4460

Oops, I will update it when commit this patch.

This revision was automatically updated to reflect the committed changes.

I realize that most reviewers were on vacation during this patch was reviewed and committed. It is a bit too haste to commit this patch. Please add the comments here if have any concern. Sorry about this.

amyk added a subscriber: amyk.Jan 2 2020, 8:30 PM

Just curious about a specific portion of your patch, so I posted a comment.

llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp
4460

Might be a silly question, but what exactly does this comment mean and why is a 16-bit immediate significant? And did you mean to say, If it is not a 16-bit imm?