[PATCH] D72518: [clang] New __attribute__((__clang_arm_mve_strict_polymorphism)).

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/include/clang/Basic/Attr.td:1473
 
+def ArmMveStrictPolymorphism : TypeAttr {
+  let Spellings = [Clang<"__clang_arm_mve_strict_polymorphism">];
----------------
Should this be a target-specific attribute?


================
Comment at: clang/include/clang/Basic/Attr.td:1474
+def ArmMveStrictPolymorphism : TypeAttr {
+  let Spellings = [Clang<"__clang_arm_mve_strict_polymorphism">];
+  let Documentation = [ArmMveStrictPolymorphismDocs];
----------------
Why does this have a `__clang` prefix? That seems a bit strange given that the attribute can be spelled `[[clang::__clang_arm_mve_strict_polymorphism]]`. I'd probably drop the `__clang_` prefix entirely.


================
Comment at: clang/include/clang/Basic/AttrDocs.td:4793
+conversion. The aim is to prevent spurious ambiguity in ARM MVE polymorphic
+intrinsics.
+
----------------
I think adding a code example of correct vs erroneous code would be useful.


================
Comment at: clang/include/clang/Basic/AttrDocs.td:4796
+However, this attribute does not prohibit lax vector conversions in contexts
+other than overloading.
+    }];
----------------
If you have an easy code example demonstrating this, that would be useful as well.


================
Comment at: clang/lib/Sema/SemaType.cpp:7548
+      ASTContext &Ctx = state.getSema().Context;
+      type = state.getAttributedType(
+          createSimpleAttr<ArmMveStrictPolymorphismAttr>(Ctx, attr), type,
----------------
So this attribute can be written on any type? I would have expected it to be limited to just types that can interact through vector operations. Does it make sense to declare, for instance, a struct having this type?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72518