HsiangKai added inline comments.
================ Comment at: clang/include/clang/Basic/AttrDocs.td:2147-2150 +This attribute can only be used to set up the aliases for certain RISC-V +C intrinsic functions; it is intended for use only inside ``riscv_*.h`` +and is not a general mechanism for declaring arbitrary aliases for +clang builtin functions. ---------------- aaron.ballman wrote: > HsiangKai wrote: > > HsiangKai wrote: > > > aaron.ballman wrote: > > > > Why a specific attribute for only one architecture rather than a more > > > > general solution that can be used for multiple architectures? We > > > > already have `__clang_arm_builtin_alias` (with the incorrect name, so I > > > > guess I see where you got the __ from) and now we're extending it. I am > > > > not certain if there's a reason why we should use a different attribute > > > > for each target though. > > > I agree with you. We should not define the attribute as target specific. > > > This patch is a quick solution to address the issue > > > https://bugs.llvm.org/show_bug.cgi?id=49962. > > > > > > I will refactor this patch and try to merge it with > > > `__clang_arm_builtin_alias` in some way. > > @aaron.ballman Could we land this patch first to reduce the test time? > > I agree with you that it is better to have one unified attribute for the > > purpose. However, if we change the attribute defined by ARM, we need to > > negotiate with them and there may be already some use cases in their users. > > I am not sure the impact of the modification. > > > > I could modify my patch to make the attribute target independent and leave > > ARM's attribute as before. What do you think? > As best I can tell, at least from the attribute that we surface for the > users, there shouldn't be much difference between the RISC-V and the ARM > attributes. They're both accepted on the same subjects, have the same > attribute arguments, and have the same semantic effect, etc. It seems like > the only difference is with semantic checking of whether the builtin is valid > for the architecture. Based on the assumption that these are effectively > doing the same thing, I'd rather we land the generic attribute so that we can > deprecate `__clang_arm_builtin_alias` in favor of the general attribute. If > we add `__clang_riscv_builtin_alias` before adding the generic attribute, > it's one more thing for us to deprecate and then remove. > > Note, I'd be totally fine if the generic attribute only handles ARM and > RISC-V builtins initially (and error on the other architectures). We can add > support for other architectures as the need arises. > > Do you think that approach makes sense? It makes sense. I will integrate D100930 into this one. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100611/new/ https://reviews.llvm.org/D100611 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
