This is an archive of the discontinued LLVM Phabricator instance.

[VE] Extract & insert vector element isel
ClosedPublic

Authored by simoll on Dec 22 2020, 4:11 AM.

Details

Summary

Isel and tests for extract_vector_elt and insert_vector_elt.

Diff Detail

Event Timeline

simoll created this revision.Dec 22 2020, 4:11 AM
simoll requested review of this revision.Dec 22 2020, 4:11 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 22 2020, 4:11 AM
simoll planned changes to this revision.Dec 23 2020, 6:46 AM
simoll updated this revision to Diff 314373.Jan 4 2021, 6:21 AM
  • Fixed custom lowering code for packed vectors and updated tests.
  • Removed unrelated BUILD_VECTOR custom lowering to insert/extract elt.
kaz7 added inline comments.Jan 4 2021, 7:50 AM
llvm/lib/Target/VE/VEISelLowering.cpp
307

The definition of BUILD_VECTOR action and other INSERT_VECTOR_ELT/EXTRACT_VECTOR_ELT actions are separated by VVP actions.

llvm/lib/Target/VE/VEInstrPatternsVec.td
18–19

Long lines.

26

You can merge these three lines.

74

defm : extract...? Missing a space.

llvm/test/CodeGen/VE/Vector/extract_elt.ll
7

It's better to use i32 signext %idx for arguments.

36

I simply don't understand why this won't cause errors...

47

Same here. i32 signext/zeroext %idx.

llvm/test/CodeGen/VE/Vector/insert_elt.ll
8

i32 signext/zeroext %idx.

simoll updated this revision to Diff 314535.Jan 5 2021, 1:49 AM
simoll marked 5 inline comments as done.
simoll edited the summary of this revision. (Show Details)

NFC

  • Use signext for arguments in tests.
  • No more than 80 chars in VEInstrPatternVec.td file.
simoll added inline comments.Jan 5 2021, 2:56 AM
llvm/lib/Target/VE/VEISelLowering.cpp
307

You mean that the setOperationAction calls for VVP are not at the top or bottom of the initVPUActions function?
If i understand correctly, why does it matter?

llvm/lib/Target/VE/VEInstrPatternsVec.td
26

Great! I suppose you mean the VBRDrl and VBRDil patterns. How do i do this? There would need to be a noop replacement for the ImmOp Operand..

llvm/test/CodeGen/VE/Vector/extract_elt.ll
36

Isel legalizes this into two v256i64 arguments, which are mapped to v0 and v1

kaz7 added inline comments.Jan 5 2021, 3:12 AM
llvm/lib/Target/VE/VEISelLowering.cpp
307

Yes. If you have a certain reason to do that, it's fine. But, at least, I would like to understand it since I may change codes also. If you are doing without any reason, I would like to ask you to organize changes in maybe next patch.

llvm/lib/Target/VE/VEInstrPatternsVec.td
26

No. What I said is merge three lines into one line like (VBRDrl (SuperRegCast $sy) i32:$vl)>; Doing that organize vbrd_elm32 and vbrd_elm64 consitently and make review easy.

llvm/test/CodeGen/VE/Vector/extract_elt.ll
36

I see. I was afraiding that compiler is generating wrong code without any warning/errors. Now, I've understood why lvs uses %v1(116) as its operands. Thanks.

86

i32 signext %idx

182

i32 signext %idx

llvm/test/CodeGen/VE/Vector/insert_elt.ll
47

i32 signext %s, or i32 zeroext %s to avoid and.

57

same here.

67

same here.

78

same here.

92

same here.

kaz7 added a comment.Jan 5 2021, 5:30 AM

A few more modificating candidates to make code better.

llvm/lib/Target/VE/VEISelLowering.cpp
2714

Op.getValueType() returns EVT. I guess using Op.getSimpleValueType() is better for this.

2717

Op.getSimpleValueType()

2744

Idx.getSimpleValueType()

2746

Val.getSimpleValueType()

2748

Val.getSimpleValueType()

simoll updated this revision to Diff 314578.Jan 5 2021, 5:40 AM
simoll marked 10 inline comments as done.

NFC

  • Hopefully attached signext to all args in tests this time
  • Adressed comments
simoll updated this revision to Diff 314582.Jan 5 2021, 5:45 AM
simoll marked 6 inline comments as done.

getSimpleValueType()

llvm/lib/Target/VE/VEISelLowering.cpp
307

Ok. This is now structured as follows:
The first loop iterates over all non-packed VTs, VVP actions come last in that loop.
The second loop iterates over all packed VTs..

kaz7 accepted this revision.Jan 5 2021, 5:51 AM

Thanks! LGTM.

This revision is now accepted and ready to land.Jan 5 2021, 5:51 AM
simoll updated this revision to Diff 315307.Jan 8 2021, 12:53 AM
  • Rebased onto main.
  • insert_vector_elt tests trigger vbrd code path because lowering of build_vector to insert_vector_elt is gone (D93759).
simoll edited the summary of this revision. (Show Details)Jan 8 2021, 12:53 AM
This revision was landed with ongoing or failed builds.Jan 8 2021, 2:47 AM
This revision was automatically updated to reflect the committed changes.