This is an archive of the discontinued LLVM Phabricator instance.

[VE] Add logical mask intrinsic instructions
ClosedPublic

Authored by kaz7 on Dec 11 2020, 1:24 AM.

Details

Summary

Add andm, orm, xorm, eqvm, nndm, negm, pcvm, lzvm, and tovm intrinsic
instructions, a few pseudo instructions to expand logical intrinsic
using VM512, a mechnism to expand such pseudo instructions, and
regression tests. Also, assign vector mask types and vector mask
register classes correctly. This is required to use VM512 registers
as function arguments.

Diff Detail

Event Timeline

kaz7 created this revision.Dec 11 2020, 1:24 AM
kaz7 requested review of this revision.Dec 11 2020, 1:25 AM

Just this one thing, else, LGTM.

llvm/lib/Target/VE/VEISelLowering.cpp
87–89 ↗(On Diff #311141)

Can we make this a separate patch?

kaz7 retitled this revision from [VE] Add logical vector intrinsic instructions to [VE] Add logical mask intrinsic instructions.Dec 12 2020, 9:36 AM
kaz7 added a comment.Dec 12 2020, 4:53 PM

Hmm, not sure why it needs. These new tests shows the correctness of code modifications enough, IMO.

llvm/lib/Target/VE/VEISelLowering.cpp
87–89 ↗(On Diff #311141)

I condider that, but it's difficult to do so. In order to do so, a regression test is required. Making such a regression test requires additional implementation of vector mask register copy function. I'm not sure what is required to implement that.

Probablly, I should ask them when you add above code. I asked tests for V64 registers but forgot to ask tests of VM registers.

Anywa, this time I add new intrinsic instructions supporting vm512 and hit this bug. You can see crashes by disabling above modifications and run new regression tests that this patch adds. Then, you can see no crash after enabling above modifications and run the identical tests again. Isn't that enough?

kaz7 updated this revision to Diff 311418.Dec 12 2020, 4:54 PM

Update patch following clang-tidy suggestions.

simoll added inline comments.Dec 14 2020, 6:53 AM
llvm/lib/Target/VE/VEISelLowering.cpp
87–89 ↗(On Diff #311141)

You can fix this in a separate patch without additional tests since it is obviously incorrect. I am asking you to do this because this patch adds new functionality first - we shouldn't bundle it up with bug fixes.

kaz7 added a comment.Dec 14 2020, 7:01 AM

Separate this bug fixes to https://reviews.llvm.org/D93212.

llvm/lib/Target/VE/VEISelLowering.cpp
87–89 ↗(On Diff #311141)

Ok. If you don't require additional test for bug fix, I can do it.

kaz7 updated this revision to Diff 311575.Dec 14 2020, 7:03 AM

Split a part of this patch into https://reviews.llvm.org/D93212.

simoll accepted this revision.Dec 14 2020, 7:28 AM
This revision is now accepted and ready to land.Dec 14 2020, 7:28 AM
This revision was landed with ongoing or failed builds.Dec 14 2020, 8:34 AM
This revision was automatically updated to reflect the committed changes.