Page MenuHomePhabricator

[AArch64][SVE] Handle svbool_t VLST <-> VLAT/GNUT conversion
ClosedPublic

Authored by junparser on Mon, Jul 19, 6:39 PM.

Details

Summary

According to https://godbolt.org/z/q5rME1naY and acle, we found that
there are different SVE conversion behaviours between clang and gcc. It turns
out that llvm does not handle SVE predicates width properly.

This patch 1) checks SVE predicates width rightly with svbool_t type.

  1. removes warning on svbool_t VLST <-> VLAT/GNUT conversion.
  2. disables VLST <-> VLAT/GNUT conversion between SVE vectors and predicates

due to different width.

Diff Detail

Event Timeline

junparser created this revision.Mon, Jul 19, 6:39 PM
junparser requested review of this revision.Mon, Jul 19, 6:39 PM
Herald added a project: Restricted Project. · View Herald TranscriptMon, Jul 19, 6:39 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
junparser edited the summary of this revision. (Show Details)Mon, Jul 19, 6:40 PM

@efriedma with this patch, all of conversion between VLST and VLAT should have same vector size(getElementType() * getElementCount()). The regression in D105097 will be fixed by using bitcast + vector.insert/extract directly

@efriedma with this patch, all of conversion between VLST and VLAT should have same vector size(getElementType() * getElementCount()). The regression in D105097 will be fixed by using bitcast + vector.insert/extract directly

OK, actually this is wrong due to vscale representation in llvm ir. However, we can still use bitcast as long as we can handle <32*i1> <64*i1>... in backend? any suggestion about this? @efriedma

paulwalker-arm added a comment.EditedTue, Jul 20, 2:57 AM

@efriedma with this patch, all of conversion between VLST and VLAT should have same vector size(getElementType() * getElementCount()). The regression in D105097 will be fixed by using bitcast + vector.insert/extract directly

I hope I've not got the wrong end of the stick here but the above is our intention. As in, Arm is looking at replacing the "via memory predicate casting" with a method that uses vector_of_i8s vector insert/extract with the necessary bitcasting. Before doing this we just had to fix up a bunch of failing INSERT_SUBVECTOR cases when it comes to the illegal types this idiom introduces.

@efriedma with this patch, all of conversion between VLST and VLAT should have same vector size(getElementType() * getElementCount()). The regression in D105097 will be fixed by using bitcast + vector.insert/extract directly

I hope I've not got the wrong end of the stick here but the above is our intention. As in, Arm is looking at replacing the "via memory predicate casting" with a method that uses vector_of_i8s vector insert/extract with the necessary bitcasting. Before doing this we just had to fix up a bunch of failing INSERT_SUBVECTOR cases when it comes to the illegal types this idiom introduces.

Good to know this.

Matt added a subscriber: Matt.Tue, Jul 20, 6:35 AM

Can you add a helper somewhere for the BT->getKind() == BuiltinType::SveBool ? getLangOpts().ArmSveVectorBits / getCharWidth() : getLangOpts().ArmSveVectorBits) pattern?

junparser updated this revision to Diff 360352.Tue, Jul 20, 8:58 PM

Address comments.

paulwalker-arm added inline comments.Wed, Jul 21, 2:01 AM
clang/lib/AST/ASTContext.cpp
8677

Out of interest is this indirection necessary? I mean we know sve predicates are exactly an eighth the size of sve vectors so why not just use 8?

junparser added inline comments.Wed, Jul 21, 3:59 AM
clang/lib/AST/ASTContext.cpp
8677

Just want to keep same style since HandleArmSveVectorBitsTypeAttr use this. 8 is make sense to me.

c-rhodes added inline comments.Wed, Jul 21, 4:04 AM
clang/lib/AST/ASTContext.cpp
8673

s/perdicate/predicate

clang/test/SemaCXX/aarch64-sve-lax-vector-conversions.cpp
13–15

unused?

17

also unused?

clang/test/SemaCXX/attr-arm-sve-vector-bits.cpp
32

s/perdicates/predicates

junparser updated this revision to Diff 360690.Wed, Jul 21, 8:39 PM

Address comments.

This revision is now accepted and ready to land.Wed, Jul 21, 9:09 PM
This revision was landed with ongoing or failed builds.Wed, Jul 21, 10:55 PM
This revision was automatically updated to reflect the committed changes.