This is an archive of the discontinued LLVM Phabricator instance.

[MVT][SVE] Add EVT strings and Type mapping

Authored by huntergr on Jun 5 2018, 5:15 AM.



Adds mappings between scalable MVT types and IR Types,
along with string printing. Fixes an issue reported by Jeroen.

Diff Detail

Event Timeline

huntergr created this revision.Jun 5 2018, 5:15 AM
This comment was removed by huntergr.

Won't you need to have printing support for the generic scalable vector ? That should come here.

huntergr updated this revision to Diff 150303.Jun 7 2018, 5:31 AM

Fixed string representation of scalable vectors which don't map to the existing defined set of MVTs.

huntergr marked an inline comment as done.Jun 7 2018, 5:31 AM
greened added inline comments.Mar 7 2019, 1:57 PM

I can't find where these are defined, but given that the IR type is <scalable v4f32> should these names reflect that? "nx" is nice and short but may be confusing in that the mapping from "scalable" to "nx" isn't necessarily obvious.

rkruppe added inline comments.Mar 7 2019, 2:05 PM

As MC layer support for SVE started landing quite a while ago, the backend types too landed back then with names matching the then-current proposal for the IR types (<n x 4 x ...>). I agree it would be nice to have the backend type names match, but that's a bunch of churn, so it shouldn't be coupled to this patch and should wait until the IR type system changes are committed in case their name changes again.

greened added inline comments.Mar 8 2019, 7:26 AM

Thanks, that makes sense to me.

simoll added a subscriber: simoll.Mar 8 2019, 9:00 AM
hsaito added a subscriber: hsaito.Mar 8 2019, 2:28 PM

What's the status of this?

huntergr updated this revision to Diff 212781.Aug 1 2019, 4:42 AM
huntergr edited the summary of this revision. (Show Details)
huntergr set the repository for this revision to rG LLVM Github Monorepo.
  • Updated to head
  • Removed MVT-specific ElementCount in favour of using the common struct
  • Added unit tests.
Herald added a project: Restricted Project. · View Herald TranscriptAug 1 2019, 4:42 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
greened added inline comments.Aug 1 2019, 12:12 PM
347 ↗(On Diff #212781)

It would be helpful to add a comment about what true means, here and below:

return VectorType::get(Type::getInt1Ty(Context), 1, /* isScalable = */true);
416 ↗(On Diff #212781)

Add a comment about what false means, similar to above.

huntergr updated this revision to Diff 213007.Aug 2 2019, 3:29 AM
  • Added inline comments to clarify boolean parameter meaning.
huntergr marked 2 inline comments as done.Aug 2 2019, 3:30 AM
troyj added a subscriber: troyj.Aug 2 2019, 8:44 AM

LGTM, thanks!

208 ↗(On Diff #213007)

This isn't a comment on this patch per se but rather a question about this code overall. Why do we have all these vector and integer cases, for SVE and non-SVE? It seems like the default case should handle them all.

I'm not at all saying you should change this. It's just an observation for a possible future NFC change from <someone>.

greened accepted this revision.Aug 2 2019, 8:58 AM
This revision is now accepted and ready to land.Aug 2 2019, 8:58 AM
This revision was automatically updated to reflect the committed changes.