[PATCH] D100611: [RISCV] Add new attribute __clang_riscv_builtin_alias for intrinsics.

Wed Apr 21 10:41:28 PDT 2021

aaron.ballman 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.
----------------
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?

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100611/new/

https://reviews.llvm.org/D100611