This is an archive of the discontinued LLVM Phabricator instance.

[SveEmitter] Implement builtins for gathers/scatters
ClosedPublic

Authored by andwar on Apr 8 2020, 8:16 AM.

Details

Summary

This patch adds builtins for:

  • regular, first-faulting and non-temporal gather loads
  • regular and non-temporal scatter stores

Depends on D76680

Diff Detail

Event Timeline

andwar created this revision.Apr 8 2020, 8:16 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 8 2020, 8:16 AM
SjoerdMeijer accepted this revision.Apr 20 2020, 6:15 AM
SjoerdMeijer added a reviewer: efriedma.

This is a big patch, but looks reasonable to me.

clang/lib/CodeGen/CGBuiltin.cpp
7482

nit: to be consistent, do this in the default clause?

7489

nit: no need for the newlines here and also below?

7586

nit: can you rephrase this comment a bit I.e. the "From ACLE we always get ..." is a bit confusing I think. You want to say that this is how the ACLE defines this, but the IR looks different. You can also specify which bit is different, because that was not immediately obvious to me.

This revision is now accepted and ready to land.Apr 20 2020, 6:15 AM
sdesmalen added inline comments.Apr 20 2020, 8:20 AM
clang/lib/CodeGen/CGBuiltin.cpp
7632

nit: This can use auto, which would make OverloadedTy a llvm::VectorType

7633

Just be aware that getVectorElementCount has been removed from Type in D77278, so you'll need to cast to VectorType and use getElementCount instead.
(fyi - in D77596 I've made getSVEType actually return a VectorType so that cast wouldn't be needed)

7666

You can remove the cast<llvm::VectorType> if you make the variable use auto.
(these comments apply equally to EmitSVEGatherLoad)

sdesmalen added inline comments.Apr 20 2020, 9:21 AM
clang/lib/CodeGen/CGBuiltin.cpp
7482

Doing that would lead to

warning: default label in switch which covers all enumeration values [-Wcovered-switch-default]
andwar updated this revision to Diff 258957.Apr 21 2020, 3:36 AM
andwar marked 5 inline comments as done.

Thank you both for reviewing!

  • Rebased on top of master
  • Address the comments
  • Made getSVEType return VectorType (this simplifies a lot of things)
andwar added inline comments.Apr 21 2020, 3:38 AM
clang/lib/CodeGen/CGBuiltin.cpp
7489

IMHO this improves readability, but I agree that that's a matter of personal preference :)

I'll keep it as is for the sake of consistency with getSVEType below.

7586

Good point, updated!

7632

Thanks, good point! I also updated getSVEType to return VectorType (so I can use auto for both).

7633

I have done the same ;)

sdesmalen accepted this revision.Apr 21 2020, 5:32 AM

LGTM! Cheers @andwar

The buildbot failures are unrelated to this patch, and locally make check-all worked fine. I'll submit this patch as is.

This revision was automatically updated to reflect the committed changes.