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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits