This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] Exploitation of xxeval instruction for AND and NAND
ClosedPublic

Authored by stefanp on Dec 1 2020, 12:37 PM.

Details

Summary

The xxeval instruction was intorduced in Power PC in Power 10.
The instruction accepts three vector registers and an immediate.
Depending on the value of the immediate the instruction can be used
to perform certain bitwise boolean operations (and, or, xor, ...) on
the given vector registers.

This patch implements the AND and NAND patterns that can be used by
the instruction.

Diff Detail

Unit TestsFailed

Event Timeline

stefanp created this revision.Dec 1 2020, 12:37 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 1 2020, 12:37 PM
stefanp requested review of this revision.Dec 1 2020, 12:37 PM
stefanp added a reviewer: Restricted Project.Dec 1 2020, 12:38 PM
bsaleil added a subscriber: bsaleil.Dec 1 2020, 1:28 PM
bsaleil added inline comments.
llvm/lib/Target/PowerPC/PPCInstrPrefix.td
2603

Should we add the equivalent functions from the ISA in comments next to the patterns that were simplified ? Like // nand(A,nor(B,C)) here.

LGTM as long as the test case is updated.

llvm/lib/Target/PowerPC/PPCInstrPrefix.td
2584

I assume that we legalize bitwise operations on vectors by promoting everything to v4i32. If so, these patterns are perfectly fine. However, some of the test cases should use different types.

nemanjai accepted this revision.Dec 1 2020, 4:45 PM

I think Baptiste's suggestion to add the expression prior to each pattern is a good idea.

This revision is now accepted and ready to land.Dec 1 2020, 4:45 PM
stefanp updated this revision to Diff 309262.Dec 3 2020, 7:57 AM

Updated the test case to use a multiclass and added a comment to each pattern.
Also added different types to one of the patterns.

stefanp updated this revision to Diff 309263.Dec 3 2020, 8:01 AM

A few of the lines in the TD file were too long so I fixed that.

bsaleil accepted this revision.Dec 4 2020, 7:55 AM

LGTM, just a minor comment regarding the multiclass.

llvm/lib/Target/PowerPC/PPCInstrPrefix.td
2565

I think we don't need a multiclass here:

class xxevalPattern <dag pattern, bits<8> imm> :                                
  Pat<(v4i32 pattern), (XXEVAL $vA, $vB, $vC, imm)> {}
2585

Then there is no need to use defm here

NeHuang accepted this revision.Dec 4 2020, 9:16 AM
NeHuang added a subscriber: NeHuang.

LGTM other than one minor can be addressed when committing.

llvm/lib/Target/PowerPC/PPCInstrPrefix.td
2565

Agree with Baptiste's point above that we can use class here

stefanp updated this revision to Diff 309640.Dec 4 2020, 1:41 PM

Changed the multiclass into class.

I will try to commit this patch on Monday if there are no further comments.

jsji added a subscriber: jsji.Dec 4 2020, 2:25 PM
jsji added inline comments.
llvm/lib/Target/PowerPC/PPCInstrPrefix.td
2569

Why xxeval needs PrefixInstrs Predicates?

2585

Can we use switch table + foreach to define all patterns?

something like:

class xxevalpat<bits<8> imm>{                                                                                                                                                 
dag pattern = !cond(!eq(imm,  1): (and v4i32:$vA, (and v4i32:$vB, v4i32:$vC)),                                                                                                                  
                        !eq(imm,  6): (and v4i32:$vA, (xor v4i32:$vB, v4i32:$vC))   
...                                                                                                            );                                                                                                                                                        }             


  foreach i = [1,6...] in {               
         def : xxevalPattern< xxevalpat<i>.pattern, i>;                                                                                                                            }       
  }
stefanp added inline comments.Dec 7 2020, 6:42 AM
llvm/lib/Target/PowerPC/PPCInstrPrefix.td
2569

It is required because xxeval is a 64 bit prefixed instruction.

2585

Hmm... I actually feel like this would make things more confusing to read.
It is another level of indirection but in the end I am still just listing pairs of immediate values and patterns that go with them. On top of that I would have to maintain a list of values in the foreach.
I think I prefer to leave this as it stands.

jsji added inline comments.Dec 7 2020, 7:56 AM
llvm/lib/Target/PowerPC/PPCInstrPrefix.td
2569

Ah.. OK. Missed that in ISA..

2585

I would have to maintain a list of values in the foreach

I think we don't, we may simple foreach 1-255, then handle the values in class.

but in the end I am still just listing pairs of immediate values and patterns that go with them.

This is just the implementation example, I was hoping that we can come up with better code that can return the pattern according to bit of imm.

But yeah, this is nice to have.

jsji accepted this revision.Dec 7 2020, 9:52 AM

Thanks , LGTM as it is per discussion.