This is an archive of the discontinued LLVM Phabricator instance.

[VP] Legalize the stride operand for EXPERIMENTAL_VP_STRIDED SDNodes
ClosedPublic

Authored by loralb on Apr 5 2022, 3:01 AM.

Details

Summary

Add promotion and expansion of integer operands for experimental_vp_strided SelectionDAG nodes; the expansion is actually just a truncation of the stride operand.

Diff Detail

Unit TestsFailed

Event Timeline

loralb created this revision.Apr 5 2022, 3:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 5 2022, 3:01 AM
loralb requested review of this revision.Apr 5 2022, 3:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 5 2022, 3:01 AM
craig.topper added inline comments.Apr 6 2022, 8:57 AM
llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp
5145

Use GetExpandedInteger and assign NewOps[OpNo] to Lo

loralb updated this revision to Diff 421471.Apr 8 2022, 3:10 AM

Changelog:

  • Address review comment
loralb marked an inline comment as done.Apr 8 2022, 3:11 AM

Looks good to me but I'd defer to @craig.topper in case I've missed something.

llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp
2340

Would this be at all nicer if it was a single assert? assert((opc == load && opno == 3) || (opno == store && opno == 4))? Having explicit if/else just filled with asserts is a little odd to me.

craig.topper added inline comments.May 10 2022, 9:37 AM
llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp
2340

+1

5138

Probably worth a comment calling out that we drop the upper half.

loralb updated this revision to Diff 431401.May 23 2022, 9:28 AM

Changelog:

  • Address review comments

Is it possible to test this?

loralb added a comment.Jun 9 2022, 6:44 AM

Is it possible to test this?

I am testing this in D121113, which indeed depends on this patch.
In order to move the tests here, I would have to duplicate the ones in D121113 both for riscv32 and riscv64 and add that revision as parent of this one.
Which approach would you suggest?

craig.topper accepted this revision.Jun 9 2022, 8:21 AM

Is it possible to test this?

I am testing this in D121113, which indeed depends on this patch.
In order to move the tests here, I would have to duplicate the ones in D121113 both for riscv32 and riscv64 and add that revision as parent of this one.
Which approach would you suggest?

Ok, we can leave them in D121113.

LGTM

This revision is now accepted and ready to land.Jun 9 2022, 8:21 AM
loralb updated this revision to Diff 444731.Jul 14 2022, 10:59 AM

Changelog:

  • Rebase

I could not apply this patch. Please rebase again so i can commit.

loralb updated this revision to Diff 445797.Jul 19 2022, 6:27 AM

Changelog:

  • Rebase

I could not apply this patch. Please rebase again so i can commit.

Rebase done

This revision was landed with ongoing or failed builds.Jul 20 2022, 1:23 AM
This revision was automatically updated to reflect the committed changes.