[PATCH] D150953: [Clang][SVE2.1] Add clang support for prototypes using svcount_t
hassnaa-arm added inline comments. Comment at: clang/include/clang/Basic/Builtins.def:42 // q -> Scalable vector, followed by the number of elements and the base type. +// Q -> AArch64 svcount_t builtin type. // E -> ext_vector, followed by the number of elements and the base type. sdesmalen wrote: > Can we make 'Q' something to mean 'target type' and then use a second letter > to clarify which exact target type it is, e.g. > > // Q -> target builtin type, followed by a character to distinguish the > builtin type > // Qa -> AArch64 svcount_t builtin type. > > That should make it more extensible for other target types in the future. Is that suggestion because maybe other targets don't support svcount ? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D150953/new/ https://reviews.llvm.org/D150953 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D150953: [Clang][SVE2.1] Add clang support for prototypes using svcount_t
hassnaa-arm added inline comments. Comment at: clang/include/clang/Basic/Builtins.def:42 // q -> Scalable vector, followed by the number of elements and the base type. +// Q -> AArch64 svcount_t builtin type. // E -> ext_vector, followed by the number of elements and the base type. sdesmalen wrote: > hassnaa-arm wrote: > > sdesmalen wrote: > > > Can we make 'Q' something to mean 'target type' and then use a second > > > letter to clarify which exact target type it is, e.g. > > > > > > // Q -> target builtin type, followed by a character to distinguish the > > > builtin type > > > // Qa -> AArch64 svcount_t builtin type. > > > > > > That should make it more extensible for other target types in the future. > > Is that suggestion because maybe other targets don't support svcount ? > Correct, `svcount_t` is an AArch64 specific type. Thanks. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D150953/new/ https://reviews.llvm.org/D150953 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D150953: [Clang][SVE2.1] Add clang support for prototypes using svcount_t
hassnaa-arm added inline comments. Comment at: clang/include/clang/Basic/arm_sve.td:56 // P: boolean +// Qa: svcount // U: unsigned sdesmalen wrote: > The typespec modifier in this file can remain `Q` rather than `Qa`. Can you > change it back? what if there is a new type that is target-specific ? At that case we will have to find a new character. But if we now use Qa, we can use Qb for the new type. Is that correct ? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D150953/new/ https://reviews.llvm.org/D150953 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D151197: [Clang][SVE2p1] Add svpsel builtins
hassnaa-arm added inline comments. Comment at: clang/include/clang/Basic/arm_sve.td:2123 + +def SVPSEL_COUNT_ALIAS_B : SInst<"svpsel_lane_c8", "}}Pmi", "Pc", MergeNone, "", [], [ImmCheck<3, ImmCheck0_15>]>; +def SVPSEL_COUNT_ALIAS_H : SInst<"svpsel_lane_c16", "}}Pmi", "Ps", MergeNone, "", [], [ImmCheck<3, ImmCheck0_7>]>; In the section of prototype modifiers: "}}Pmi" According to the used modifiers, I see that they refer to 4 parameters, while in the testing file I see the function takes 3 parameters only. Isn't that 'i' modifier extra ? The same case for all the builtins. Am I correct ? Comment at: clang/lib/CodeGen/CGBuiltin.cpp:9633-9635 +bool IsSVCount = isa(Ops[0]->getType()); +assert(((!IsSVCount || cast(Ops[0]->getType())->getName() == + "aarch64.svcount")) && for the case of sve::BI__builtin_sve_svpsel_lane_b8, what is the expected value of IsSVCount ? and how the assertion statement didn't assert for the check of : (cast(Ops[0]->getType())->getName() == "aarch64.svcount")) how is the parameter type considered as aarch64.svcount ? Comment at: clang/lib/CodeGen/CGBuiltin.cpp:9642 +Function *CastToSVCountF = +CGM.getIntrinsic(Intrinsic::aarch64_sve_convert_from_svbool, SVCountTy); + Isn't the type of 'SVCountTy' = 'aarch64.svcount' as a result of the statement at line 9637 ? So why do we need to aarch64_sve_convert_from_svbool while it's not svbool. Am I missing something ? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D151197/new/ https://reviews.llvm.org/D151197 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D151197: [Clang][SVE2p1] Add svpsel builtins
hassnaa-arm added inline comments. Comment at: clang/lib/CodeGen/CGBuiltin.cpp:9633-9635 +bool IsSVCount = isa(Ops[0]->getType()); +assert(((!IsSVCount || cast(Ops[0]->getType())->getName() == + "aarch64.svcount")) && hassnaa-arm wrote: > for the case of sve::BI__builtin_sve_svpsel_lane_b8, > what is the expected value of IsSVCount ? and how the assertion statement > didn't assert for the check of : > (cast(Ops[0]->getType())->getName() == >"aarch64.svcount")) > how is the parameter type considered as aarch64.svcount ? Hi Carol, I understood that part, ignore my comment. Comment at: clang/lib/CodeGen/CGBuiltin.cpp:9642 +Function *CastToSVCountF = +CGM.getIntrinsic(Intrinsic::aarch64_sve_convert_from_svbool, SVCountTy); + hassnaa-arm wrote: > Isn't the type of 'SVCountTy' = 'aarch64.svcount' as a result of the > statement at line 9637 ? > So why do we need to aarch64_sve_convert_from_svbool while it's not svbool. > Am I missing something ? I understood that part, please ignore my comment. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D151197/new/ https://reviews.llvm.org/D151197 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
