Page MenuHomePhabricator

[VP] Propagate align parameter attr on VP gather/scatter to ISel
ClosedPublic

Authored by frasercrmck on Nov 23 2021, 2:06 AM.

Details

Summary

This patch fixes a case where the 'align' parameter attribute on the
pointer operands to llvm.vp.gather and llvm.vp.scatter was being dropped
during the conversion to the SelectionDAG. The default alignment equal
to the ABI type alignment of the vector type was kept. It also updates
the documentation to reflect the fact that the parameter attribute is
now properly supported.

The default alignment of these intrinsics was previously documented as
being equal to the ABI alignment of the *scalar* type, when in fact that
wasn't the case: the ABI alignment of the vector type was used instead.
This has also been fixed in this patch.

Diff Detail

Event Timeline

frasercrmck created this revision.Nov 23 2021, 2:06 AM
frasercrmck requested review of this revision.Nov 23 2021, 2:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 23 2021, 2:06 AM
This revision is now accepted and ready to land.Nov 24 2021, 11:36 AM
  • rebase and use 'operand' over 'argument'
simoll accepted this revision.Nov 30 2021, 9:39 AM

LGTM

simoll added a comment.Dec 6 2021, 3:09 AM

What's our status with this patch?
I know we have been discussing among VP folks to get this approved by somebody who had worked on align before.

What's our status with this patch?
I know we have been discussing among VP folks to get this approved by somebody who had worked on align before.

It's in a stack on top of another patch that hasn't yet been approved. I'm also open to finding someone more closely related to align and/or IR semantics but haven't done the work to find out who that might be.

Aha I just found D87304 - maybe @jdoerfert @fhahn @anna @dantrushin would be interested in this patch.

anna added a comment.Dec 6 2021, 7:02 AM

Aha I just found D87304 - maybe @jdoerfert @fhahn @anna @dantrushin would be interested in this patch.

Hi Fraser,
Yes, this definitely unblocks some downstream only changes we had (basically partially reverting D87304) since we wanted to apply align to vector of pointers. I believe @jdoerfert wanted to see this applied to (at least one) other attribute apart from align.
The LangRef update looks good and clear to me.

I think having this as a first step is good unblocks our use case downstream as well as yours. There were some IR compatibility changes landed in follow-up patch, attached to D87304. Are those relevant here?

anna requested changes to this revision.Dec 6 2021, 7:08 AM

Actually looking at the patch again, you're mixing two concepts here:

  1. fixing the LangRef and Attributes.cpp for align attribute on vector of pointers should be done as an independent patch. It is orthogonal to the SelectionDAGBuilder code and avoids the false dependency created by the parent patch linked to this patch.
  2. ABI intrinsics and SelectionDAG builder changes can depend on the parent patch.

For #1, You need couple of tests for the attributes change and at least one in Verifier.

This revision now requires changes to proceed.Dec 6 2021, 7:08 AM
frasercrmck retitled this revision from [IR][VP] Extend the align param attr to vectors of pointers to [VP] Propagate align parameter attr on VP gather/scatter to ISel .Dec 6 2021, 8:36 AM
frasercrmck edited the summary of this revision. (Show Details)
arsenm added a subscriber: arsenm.Dec 6 2021, 8:39 AM

Should also update IRTranslator

Should also update IRTranslator

Sorry, I'm not best versed in GlobalISel. Just to check, do you mean for VP intrinsics? Or for the align parameter being applied to vectors of pointers (D115161)?

frasercrmck retitled this revision from [VP] Propagate align parameter attr on VP gather/scatter to ISel to [VP] Propagate align parameter attr on VP gather/scatter to ISel.
  • rebase, fix conflicts
  • rebase

@anna are you happy to unblock this review?

anna resigned from this revision.Tue, Jan 18, 9:00 AM

Looks like the comments have been addressed regarding the split. Thank you! Moving myself out of blocking state.

This revision is now accepted and ready to land.Tue, Jan 18, 9:00 AM
This revision was landed with ongoing or failed builds.Tue, Jan 18, 9:43 AM
This revision was automatically updated to reflect the committed changes.