[PATCH] D77735: [SveEmitter] Implement builtins for gathers/scatters
andwar added a comment. The buildbot failures are unrelated to this patch, and locally `make check-all` worked fine. I'll submit this patch as is. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77735/new/ https://reviews.llvm.org/D77735 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D77735: [SveEmitter] Implement builtins for gathers/scatters
sdesmalen accepted this revision. sdesmalen added a comment. LGTM! Cheers @andwar Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77735/new/ https://reviews.llvm.org/D77735 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D77735: [SveEmitter] Implement builtins for gathers/scatters
andwar added inline comments.
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:7458
+llvm_unreachable("Invalid SVETypeFlag!");
+
+ case SVETypeFlags::EltTyInt8:
SjoerdMeijer wrote:
> nit: no need for the newlines here and also below?
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.
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:7555
+
+ // From ACLE we always get . This might be incompatible with
+ // the actual type being loaded. Cast accordingly.
SjoerdMeijer wrote:
> 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.
>
Good point, updated!
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:7601
+ llvm::Type *SrcDataTy = getSVEType(TypeFlags);
+ llvm::Type *OverloadedTy = llvm::VectorType::get(
+ SVEBuiltinMemEltTy(TypeFlags), SrcDataTy->getVectorElementCount());
sdesmalen wrote:
> nit: This can use `auto`, which would make OverloadedTy a `llvm::VectorType`
Thanks, good point! I also updated `getSVEType` to return `VectorType` (so I
can use `auto` for both).
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:7602
+ llvm::Type *OverloadedTy = llvm::VectorType::get(
+ SVEBuiltinMemEltTy(TypeFlags), SrcDataTy->getVectorElementCount());
+
sdesmalen wrote:
> 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)
I have done the same ;)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D77735/new/
https://reviews.llvm.org/D77735
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D77735: [SveEmitter] Implement builtins for gathers/scatters
sdesmalen added inline comments.
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:7451
+ }
+ llvm_unreachable("Unknown MemEltType");
+}
SjoerdMeijer wrote:
> nit: to be consistent, do this in the default clause?
Doing that would lead to
warning: default label in switch which covers all enumeration values
[-Wcovered-switch-default]
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D77735/new/
https://reviews.llvm.org/D77735
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D77735: [SveEmitter] Implement builtins for gathers/scatters
sdesmalen added inline comments. Comment at: clang/lib/CodeGen/CGBuiltin.cpp:7601 + llvm::Type *SrcDataTy = getSVEType(TypeFlags); + llvm::Type *OverloadedTy = llvm::VectorType::get( + SVEBuiltinMemEltTy(TypeFlags), SrcDataTy->getVectorElementCount()); nit: This can use `auto`, which would make OverloadedTy a `llvm::VectorType` Comment at: clang/lib/CodeGen/CGBuiltin.cpp:7602 + llvm::Type *OverloadedTy = llvm::VectorType::get( + SVEBuiltinMemEltTy(TypeFlags), SrcDataTy->getVectorElementCount()); + 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) Comment at: clang/lib/CodeGen/CGBuiltin.cpp:7635 + // the actual type being stored. Cast accordingly. + Ops[1] = EmitSVEPredicateCast(Ops[1], cast(OverloadedTy)); + You can remove the `cast` if you make the variable use `auto`. (these comments apply equally to EmitSVEGatherLoad) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77735/new/ https://reviews.llvm.org/D77735 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D77735: [SveEmitter] Implement builtins for gathers/scatters
SjoerdMeijer accepted this revision.
SjoerdMeijer added a reviewer: efriedma.
SjoerdMeijer added a comment.
This revision is now accepted and ready to land.
This is a big patch, but looks reasonable to me.
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:7451
+ }
+ llvm_unreachable("Unknown MemEltType");
+}
nit: to be consistent, do this in the default clause?
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:7458
+llvm_unreachable("Invalid SVETypeFlag!");
+
+ case SVETypeFlags::EltTyInt8:
nit: no need for the newlines here and also below?
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:7555
+
+ // From ACLE we always get . This might be incompatible with
+ // the actual type being loaded. Cast accordingly.
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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D77735/new/
https://reviews.llvm.org/D77735
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
