[PATCH] D48617: [Builtins][Attributes][X86] Tag all X86 builtins with their required vector width. Add a min_vector_width function attribute and tag all x86 instrinsics with it.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: include/clang/Basic/Attr.td:1949
+  let Args = [UnsignedArgument<"VectorWidth">];
+  let Subjects = SubjectList<[Function], ErrorDiag>;
+  let Documentation = [Undocumented];
----------------
craig.topper wrote:
> aaron.ballman wrote:
> > Should this apply to Objective-C methods? What about other function-like interfaces such as function pointers?
> I think maybe it should apply to objective-C. but I"m not sure because it doesn't look like the target attribute applies there?
I don't have strong opinions on the question, I just wasn't sure if this attribute would be something an ObjC method would want to make use of. If that's unlikely, it's reasonable to leave it off until a use case appears.


================
Comment at: include/clang/Basic/AttrDocs.td:1502
+  let Content = [{
+clang support the ``__attribute__((min_vector_width(width)))`` attribute. This
+attribute may be attached to a function and informs that backend that this
----------------
clang support -> Clang supports


================
Comment at: include/clang/Basic/AttrDocs.td:1503
+clang support the ``__attribute__((min_vector_width(width)))`` attribute. This
+attribute may be attached to a function and informs that backend that this
+function desires vectors of at least this width to be generated. Target specific
----------------
that backend -> the backend


================
Comment at: include/clang/Basic/AttrDocs.td:1504
+attribute may be attached to a function and informs that backend that this
+function desires vectors of at least this width to be generated. Target specific
+maximum vector widths still apply. This is attribute it meant to be a hint to
----------------
Target specific -> Target-specific


================
Comment at: include/clang/Basic/AttrDocs.td:1505
+function desires vectors of at least this width to be generated. Target specific
+maximum vector widths still apply. This is attribute it meant to be a hint to
+control target heuristics that may generate narrower vectors than what the
----------------
"Apply" how?

I don't see any logic that diagnoses the situation where the user requests something larger than the maximum vector width or smaller than the minimum vector width supported by the target. I would expect the attribute to be diagnosed and dropped in that case; is there a reason not to do that?

This is attribute it -> This attribute is


================
Comment at: include/clang/Basic/AttrDocs.td:1511
+vectors to be limited to using 256-bit vectors to avoid frequency penalties.
+This attribute can be used to override this behavior on certain functions.
+}];
----------------
This suggests to me that the user can override the attribute on the builtins if you want a different behavior; is that correct?


================
Comment at: lib/Basic/Builtins.cpp:119-122
+
+  assert(::strchr(WidthPos, ':') &&
+         "Vector width specifier must be end with a ':'");
+  return ::strtol(WidthPos, nullptr, 10);
----------------
I think you want to use the ending position from `strtol()` to assert that it ended on the : and not some other random character. e.g., I think this will parse just fine: `V:1e28:` and run into issues elsewhere.


================
Comment at: lib/CodeGen/CodeGenFunction.cpp:449
+
+  // Add the required-vector-width attribute
+  if (LargestVectorWidth != 0)
----------------
Missing full stop at the end of the comment.


================
Comment at: lib/CodeGen/CodeGenFunction.cpp:1198
+  if (CurFuncDecl)
+    if (auto *VecWidth = CurFuncDecl->getAttr<MinVectorWidthAttr>())
+      LargestVectorWidth = VecWidth->getVectorWidth();
----------------
`const auto *`


================
Comment at: lib/CodeGen/CodeGenFunction.h:1466
 
+  /// Largest vector with used in ths function. Will be used to create a
+  /// function attribute.
----------------
with -> width


https://reviews.llvm.org/D48617