[PATCH] D105384: [NVPTX, CUDA] Add .and.popc variant of the b1 MMA instruction.

[Steffen Larsen via Phabricator via cfe-commits]

steffenlarsen accepted this revision.
steffenlarsen added a comment.

LGTM! Are the test failures related to this, you reckon?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105384

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D107054: [Clang][CUDA] Add descriptors, mappings, and features for missing CUDA and PTX versions

[Steffen Larsen via Phabricator via cfe-commits]

steffenlarsen created this revision.
steffenlarsen added a reviewer: tra.
Herald added subscribers: dexonsmith, hiraditya, yaxunl, jholewinski.
steffenlarsen requested review of this revision.
Herald added projects: clang, LLVM.
Herald added subscribers: llvm-commits, cfe-commits.

Adds missing descriptors and mappings for CUDA 11.4 and older. Also adds 
missing sub-target features for PTX 7.4 and older.

Authored-by: Steffen Larsen 


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D107054

Files:
  clang/include/clang/Basic/BuiltinsNVPTX.def
  clang/include/clang/Basic/Cuda.h
  clang/lib/Basic/Cuda.cpp
  clang/lib/Basic/Targets/NVPTX.cpp
  clang/lib/Driver/ToolChains/Cuda.cpp
  llvm/lib/Target/NVPTX/NVPTX.td
  llvm/lib/Target/NVPTX/NVPTXInstrInfo.td

Index: llvm/lib/Target/NVPTX/NVPTXInstrInfo.td
===
--- llvm/lib/Target/NVPTX/NVPTXInstrInfo.td
+++ llvm/lib/Target/NVPTX/NVPTXInstrInfo.td
@@ -147,6 +147,9 @@
 def hasPTX65 : Predicate<"Subtarget->getPTXVersion() >= 65">;
 def hasPTX70 : Predicate<"Subtarget->getPTXVersion() >= 70">;
 def hasPTX71 : Predicate<"Subtarget->getPTXVersion() >= 71">;
+def hasPTX72 : Predicate<"Subtarget->getPTXVersion() >= 72">;
+def hasPTX73 : Predicate<"Subtarget->getPTXVersion() >= 73">;
+def hasPTX74 : Predicate<"Subtarget->getPTXVersion() >= 74">;
 
 def hasSM30 : Predicate<"Subtarget->getSmVersion() >= 30">;
 def hasSM70 : Predicate<"Subtarget->getSmVersion() >= 70">;
Index: llvm/lib/Target/NVPTX/NVPTX.td
===
--- llvm/lib/Target/NVPTX/NVPTX.td
+++ llvm/lib/Target/NVPTX/NVPTX.td
@@ -89,6 +89,10 @@
  "Use PTX version 7.1">;
 def PTX72 : SubtargetFeature<"ptx72", "PTXVersion", "72",
  "Use PTX version 7.2">;
+def PTX73 : SubtargetFeature<"ptx73", "PTXVersion", "73",
+ "Use PTX version 7.3">;
+def PTX74 : SubtargetFeature<"ptx74", "PTXVersion", "74",
+ "Use PTX version 7.4">;
 
 //===--===//
 // NVPTX supported processors.
Index: clang/lib/Driver/ToolChains/Cuda.cpp
===
--- clang/lib/Driver/ToolChains/Cuda.cpp
+++ clang/lib/Driver/ToolChains/Cuda.cpp
@@ -77,6 +77,12 @@
 return CudaVersion::CUDA_110;
   if (raw_version < 11020)
 return CudaVersion::CUDA_111;
+  if (raw_version < 11030)
+return CudaVersion::CUDA_112;
+  if (raw_version < 11040)
+return CudaVersion::CUDA_113;
+  if (raw_version < 11050)
+return CudaVersion::CUDA_114;
   return CudaVersion::LATEST;
 }
 
@@ -131,7 +137,9 @@
   SmallVector Candidates;
 
   // In decreasing order so we prefer newer versions to older versions.
-  std::initializer_list Versions = {"8.0", "7.5", "7.0"};
+  std::initializer_list Versions = {
+  "11.4", "11.3", "11.2", "11.1", "10.2", "10.1", "10.0",
+  "9.2",  "9.1",  "9.0",  "8.0",  "7.5",  "7.0"};
   auto &FS = D.getVFS();
 
   if (Args.hasArg(clang::driver::options::OPT_cuda_path_EQ)) {
@@ -720,6 +728,8 @@
   case CudaVersion::CUDA_##CUDA_VER:   \
 PtxFeature = "+ptx" #PTX_VER;  \
 break;
+CASE_CUDA_VERSION(114, 74);
+CASE_CUDA_VERSION(113, 73);
 CASE_CUDA_VERSION(112, 72);
 CASE_CUDA_VERSION(111, 71);
 CASE_CUDA_VERSION(110, 70);
Index: clang/lib/Basic/Targets/NVPTX.cpp
===
--- clang/lib/Basic/Targets/NVPTX.cpp
+++ clang/lib/Basic/Targets/NVPTX.cpp
@@ -45,6 +45,8 @@
 if (!Feature.startswith("+ptx"))
   continue;
 PTXVersion = llvm::StringSwitch(Feature)
+ .Case("+ptx74", 74)
+ .Case("+ptx73", 73)
  .Case("+ptx72", 72)
  .Case("+ptx71", 71)
  .Case("+ptx70", 70)
Index: clang/lib/Basic/Cuda.cpp
===
--- clang/lib/Basic/Cuda.cpp
+++ clang/lib/Basic/Cuda.cpp
@@ -36,6 +36,10 @@
 return "11.1";
   case CudaVersion::CUDA_112:
 return "11.2";
+  case CudaVersion::CUDA_113:
+return "11.3";
+  case CudaVersion::CUDA_114:
+return "11.4";
   }
   llvm_unreachable("invalid enum");
 }
@@ -54,6 +58,8 @@
   .Case("11.0", CudaVersion::CUDA_110)
   .Case("11.1", CudaVersion::CUDA_111)
   .Case("11.2", CudaVersion::CUDA_112)
+  .Case("11.3", CudaVersion::CUDA_113)
+  .Case("11.4", CudaVersion::CUDA_114)
   .Default(CudaVersion::UNKNOWN);
 }
 
@@ -227,6 +233,10 @@
 return CudaVersion::CUDA_111;
   case 112:
 return CudaVersion::CUDA_112;
+  case 113:
+return CudaVersion::CUDA_113;
+  case 114:
+return CudaVersion::CUDA_114;
   default:
 return CudaVersion::UNKNOWN;
   }
Index: clang/include/cl

[PATCH] D114439: [Annotation] Allow parameter pack expansions and initializer lists in annotate attribute

[Steffen Larsen via Phabricator via cfe-commits]

steffenlarsen updated this revision to Diff 405820.
steffenlarsen added a comment.

Adjusted for comments and introduced common handling for creating attributes 
with delayed arguments.


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

https://reviews.llvm.org/D114439

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/include/clang/Parse/Parser.h
  clang/include/clang/Sema/ParsedAttr.h
  clang/include/clang/Sema/Sema.h
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Parse/ParseExpr.cpp
  clang/lib/Sema/ParsedAttr.cpp
  clang/lib/Sema/SemaAttr.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/Parser/cxx0x-attributes.cpp
  clang/test/Sema/annotate.c
  clang/test/SemaCXX/attr-annotate.cpp
  clang/test/SemaTemplate/attributes.cpp
  clang/utils/TableGen/ClangAttrEmitter.cpp

Index: clang/utils/TableGen/ClangAttrEmitter.cpp
===
--- clang/utils/TableGen/ClangAttrEmitter.cpp
+++ clang/utils/TableGen/ClangAttrEmitter.cpp
@@ -202,9 +202,9 @@
 bool Fake;
 
   public:
-Argument(const Record &Arg, StringRef Attr)
-: lowerName(std::string(Arg.getValueAsString("Name"))),
-  upperName(lowerName), attrName(Attr), isOpt(false), Fake(false) {
+Argument(StringRef Arg, StringRef Attr)
+: lowerName(std::string(Arg)), upperName(lowerName), attrName(Attr),
+  isOpt(false), Fake(false) {
   if (!lowerName.empty()) {
 lowerName[0] = std::tolower(lowerName[0]);
 upperName[0] = std::toupper(upperName[0]);
@@ -215,6 +215,8 @@
   if (lowerName == "interface")
 lowerName = "interface_";
 }
+Argument(const Record &Arg, StringRef Attr)
+: Argument(Arg.getValueAsString("Name"), Attr) {}
 virtual ~Argument() = default;
 
 StringRef getLowerName() const { return lowerName; }
@@ -666,6 +668,11 @@
   ArgName(getLowerName().str() + "_"), ArgSizeName(ArgName + "Size"),
   RangeName(std::string(getLowerName())) {}
 
+VariadicArgument(StringRef Arg, StringRef Attr, std::string T)
+: Argument(Arg, Attr), Type(std::move(T)),
+  ArgName(getLowerName().str() + "_"), ArgSizeName(ArgName + "Size"),
+  RangeName(std::string(getLowerName())) {}
+
 const std::string &getType() const { return Type; }
 const std::string &getArgName() const { return ArgName; }
 const std::string &getArgSizeName() const { return ArgSizeName; }
@@ -688,6 +695,18 @@
  << "); }\n";
 }
 
+void writeSetter(raw_ostream &OS) const {
+  OS << "  void set" << getUpperName() << "(ASTContext &Ctx, ";
+  writeCtorParameters(OS);
+  OS << ") {\n";
+  OS << "" << ArgSizeName << " = " << getUpperName() << "Size;\n";
+  OS << "" << ArgName << " = new (Ctx, 16) " << getType() << "["
+ << ArgSizeName << "];\n";
+  OS << "  ";
+  writeCtorBody(OS);
+  OS << "  }\n";
+}
+
 void writeCloneArgs(raw_ostream &OS) const override {
   OS << ArgName << ", " << ArgSizeName;
 }
@@ -1169,6 +1188,9 @@
   : VariadicArgument(Arg, Attr, "Expr *")
 {}
 
+VariadicExprArgument(StringRef ArgName, StringRef Attr)
+: VariadicArgument(ArgName, Attr, "Expr *") {}
+
 void writeASTVisitorTraversal(raw_ostream &OS) const override {
   OS << "  {\n";
   OS << "" << getType() << " *I = A->" << getLowerName()
@@ -2138,6 +2160,11 @@
   }
 }
 
+static bool isTypeArgument(const Record *Arg) {
+  return !Arg->getSuperClasses().empty() &&
+ Arg->getSuperClasses().back().first->getName() == "TypeArgument";
+}
+
 /// Emits the first-argument-is-type property for attributes.
 static void emitClangAttrTypeArgList(RecordKeeper &Records, raw_ostream &OS) {
   OS << "#if defined(CLANG_ATTR_TYPE_ARG_LIST)\n";
@@ -2149,7 +2176,7 @@
 if (Args.empty())
   continue;
 
-if (Args[0]->getSuperClasses().back().first->getName() != "TypeArgument")
+if (!isTypeArgument(Args[0]))
   continue;
 
 // All these spellings take a single type argument.
@@ -2179,7 +2206,7 @@
   OS << "#endif // CLANG_ATTR_ARG_CONTEXT_LIST\n\n";
 }
 
-static bool isIdentifierArgument(Record *Arg) {
+static bool isIdentifierArgument(const Record *Arg) {
   return !Arg->getSuperClasses().empty() &&
 llvm::StringSwitch(Arg->getSuperClasses().back().first->getName())
 .Case("IdentifierArgument", true)
@@ -2188,7 +2215,7 @@
 .Default(false);
 }
 
-static bool isVariadicIdentifierArgument(Record *Arg) {
+static bool isVariadicIdentifierArgument(const Record *Arg) {
   return !Arg->getSuperClasses().empty() &&
  llvm::StringSwitch(
  Arg->getSuperClasses().back().first->getName())
@@ -2264,6 +2291,26 @@
   OS << "#endif // CLANG_ATTR_THIS_ISA_IDENTIFIER_ARG_LIST\n\n";
 }
 
+static void emitClangAttrAcceptsExprPack(RecordKeeper &Records,
+  

[PATCH] D114439: [Annotation] Allow parameter pack expansions and initializer lists in annotate attribute

[Steffen Larsen via Phabricator via cfe-commits]

steffenlarsen marked 3 inline comments as done.
steffenlarsen added inline comments.



Comment at: clang/lib/Parse/ParseDecl.cpp:421-424
+  // Parse variadic identifier arg. This can either consume identifiers or
+  // expressions.
+  // FIXME: Variadic identifier args do not currently support parameter
+  //packs.

aaron.ballman wrote:
> steffenlarsen wrote:
> > aaron.ballman wrote:
> > > (Might need to re-flow comments to 80 col.) I don't think this is a FIXME 
> > > so much as a "this just doesn't work like that" situation.
> > I think it makes sense to have it be a FIXME because in theory it could be 
> > possible to have expression parameter packs expanded in an identifier list 
> > as it accepts expressions. I have reworded it slightly. Do you think this 
> > is better?
> Maybe we're thinking about identifier lists differently. We only have two 
> attributes that use those (`cpu_specific` and `cpu_dispatch`) and in both 
> cases (and all cases I would expect), what's being received is effectively a 
> list of enumerators (not enumerators in the C or C++ sense) that could not be 
> mixed with expressions. Expressions would go through sema and do all the 
> usual lookup work to turn them into a value, but these are not real objects 
> and so the lookup would fail for them. e.g., we're not going to be able to 
> support something like: `[[clang::cpu_specific(generic, pentium, Ts..., 
> atom)]]`. So I don't think there's anything here to be fixed (I prefer my 
> comment formulation as that makes it more clear).
I see what you mean. I have applied your wording instead.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4203
+  if (AllArgs.size() && AllArgs[0]->isValueDependent()) {
+auto *Attr = AnnotateAttr::CreateWithDelayedArgs(
+S.getASTContext(), AllArgs.data(), AllArgs.size(), AL);

aaron.ballman wrote:
> erichkeane wrote:
> > aaron.ballman wrote:
> > > erichkeane wrote:
> > > > I would like @aaron.ballman to comment on this, but I think we probably 
> > > > want this case to be covered in the top of `HandleDeclAttr`, which 
> > > > would mean in the 'not all values filled' case, we skip the 
> > > > 'handleAnnotateAttr'.  
> > > > 
> > > > WDYT Aaron?  The downside is that this function couldn't check a 
> > > > 'partially filled in' attribute, but it would make us that much closer 
> > > > to this flag being a very simple thing to opt into.
> > > Do you mean `ProcessDeclAttribute()`? I don't think we should have 
> > > attribute-specific logic in there, but are you thinking of something more 
> > > general than that (I'm not seeing how the suggestion makes the flag 
> > > easier to opt into)?
> > Ah, yes, thats what I mean.  The question for ME is whether there should be 
> > a generic "this supports variadic pack, so check to see if all the named 
> > non-expr arguments are fill-in-able.  If not, do the 'delayed' version.
> > 
> > This would mean that HandleAnnotateAttr NEVER sees the 
> > "CreateWithDelayedArgs" case.
> Thanks for clarifying -- yes, I think that would be preferable if it works 
> out in a clean, generic way. I'd be fine with tablegen emitting something 
> else (if necessary) to help generalize it.
`handleAnnotateAttr` is now oblivious to the concept of "delayed arguments". 
Instead tablegen generates a common handle function 
(`handleAttrWithDelayedArgs`) which will, based on the given `ParsedAttr` that 
supports delayed arguments, create and add the corresponding attribute with 
delayed arguments by calling the corresponding `CreateWithDelayedArgs`. The 
need for delaying arguments is decided as described in 
`MustDelayAttributeArguments`.

Is this approximately what you were thinking?



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4179-4182
+  if (AllArgs.size() < 1) {
+Diag(CI.getLoc(), diag::err_attribute_too_few_arguments) << CI << 1;
+return;
+  }

aaron.ballman wrote:
> Please double-check that the call to `checkCommonAttributeFeatures()` from 
> `ProcessDeclAttribute()` doesn't already handle this for you. If it does, 
> then I think this can be replaced with `assert(!AllArgs.empty() && "expected 
> at least one argument");`
It does go through `checkCommonAttributeFeatures` but (as of the recent 
changes) it will skip size-checks if arguments are delayed as a pack expansion 
could potentially populate the seemingly missing expressions after template 
instantiation for some attribute.
For annotate we could also have a pack as the only expression, which would then 
evaluate to an empty list of expressions. Since this path is also taken by 
`instantiateDependentAnnotationAttr` if there are delayed args. In reality it 
is only really needed after template instantiations, given as you said 
`checkCommonAttributeFeatures` will do the checking in the other case, but I 
personally think it is cleaner to have it here. If you disagree I will move

[PATCH] D114439: [Annotation] Allow parameter pack expansions and initializer lists in annotate attribute

[Steffen Larsen via Phabricator via cfe-commits]

steffenlarsen marked 2 inline comments as done.
steffenlarsen added inline comments.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:8153
+
+  bool AttrHasVariadicArg = AL.hasVariadicArg();
+  unsigned AttrNumArgs = AL.getNumArgMembers();

erichkeane wrote:
> This still doesn't work if the VariadicExprArgument isn't last, right?  Do we 
> ensure that is the case in clang-attr-emitter?
Good point. I don't think there's a check as there are select few attributes 
that use multiple variadic (`OMPDeclareSimdDecl` and `OMPDeclareVariant` are 
the only ones, I think.)

Since I don't think it's safe to check for all, should I make a check similar 
to the one for type/identifier arguments in attributes marked 
`AcceptsExprPack`? Would that suffice?


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

https://reviews.llvm.org/D114439

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114439: [Annotation] Allow parameter pack expansions and initializer lists in annotate attribute

[Steffen Larsen via Phabricator via cfe-commits]

steffenlarsen updated this revision to Diff 405963.
steffenlarsen added a comment.

Added check restricting variadic expression arguments to the last argument of 
attributes set to support parameter packs.
Added release notes.


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

https://reviews.llvm.org/D114439

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/include/clang/Parse/Parser.h
  clang/include/clang/Sema/ParsedAttr.h
  clang/include/clang/Sema/Sema.h
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Parse/ParseExpr.cpp
  clang/lib/Sema/ParsedAttr.cpp
  clang/lib/Sema/SemaAttr.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/Parser/cxx0x-attributes.cpp
  clang/test/Sema/annotate.c
  clang/test/SemaCXX/attr-annotate.cpp
  clang/test/SemaTemplate/attributes.cpp
  clang/utils/TableGen/ClangAttrEmitter.cpp

Index: clang/utils/TableGen/ClangAttrEmitter.cpp
===
--- clang/utils/TableGen/ClangAttrEmitter.cpp
+++ clang/utils/TableGen/ClangAttrEmitter.cpp
@@ -202,9 +202,9 @@
 bool Fake;
 
   public:
-Argument(const Record &Arg, StringRef Attr)
-: lowerName(std::string(Arg.getValueAsString("Name"))),
-  upperName(lowerName), attrName(Attr), isOpt(false), Fake(false) {
+Argument(StringRef Arg, StringRef Attr)
+: lowerName(std::string(Arg)), upperName(lowerName), attrName(Attr),
+  isOpt(false), Fake(false) {
   if (!lowerName.empty()) {
 lowerName[0] = std::tolower(lowerName[0]);
 upperName[0] = std::toupper(upperName[0]);
@@ -215,6 +215,8 @@
   if (lowerName == "interface")
 lowerName = "interface_";
 }
+Argument(const Record &Arg, StringRef Attr)
+: Argument(Arg.getValueAsString("Name"), Attr) {}
 virtual ~Argument() = default;
 
 StringRef getLowerName() const { return lowerName; }
@@ -666,6 +668,11 @@
   ArgName(getLowerName().str() + "_"), ArgSizeName(ArgName + "Size"),
   RangeName(std::string(getLowerName())) {}
 
+VariadicArgument(StringRef Arg, StringRef Attr, std::string T)
+: Argument(Arg, Attr), Type(std::move(T)),
+  ArgName(getLowerName().str() + "_"), ArgSizeName(ArgName + "Size"),
+  RangeName(std::string(getLowerName())) {}
+
 const std::string &getType() const { return Type; }
 const std::string &getArgName() const { return ArgName; }
 const std::string &getArgSizeName() const { return ArgSizeName; }
@@ -688,6 +695,18 @@
  << "); }\n";
 }
 
+void writeSetter(raw_ostream &OS) const {
+  OS << "  void set" << getUpperName() << "(ASTContext &Ctx, ";
+  writeCtorParameters(OS);
+  OS << ") {\n";
+  OS << "" << ArgSizeName << " = " << getUpperName() << "Size;\n";
+  OS << "" << ArgName << " = new (Ctx, 16) " << getType() << "["
+ << ArgSizeName << "];\n";
+  OS << "  ";
+  writeCtorBody(OS);
+  OS << "  }\n";
+}
+
 void writeCloneArgs(raw_ostream &OS) const override {
   OS << ArgName << ", " << ArgSizeName;
 }
@@ -1169,6 +1188,9 @@
   : VariadicArgument(Arg, Attr, "Expr *")
 {}
 
+VariadicExprArgument(StringRef ArgName, StringRef Attr)
+: VariadicArgument(ArgName, Attr, "Expr *") {}
+
 void writeASTVisitorTraversal(raw_ostream &OS) const override {
   OS << "  {\n";
   OS << "" << getType() << " *I = A->" << getLowerName()
@@ -2138,6 +2160,11 @@
   }
 }
 
+static bool isTypeArgument(const Record *Arg) {
+  return !Arg->getSuperClasses().empty() &&
+ Arg->getSuperClasses().back().first->getName() == "TypeArgument";
+}
+
 /// Emits the first-argument-is-type property for attributes.
 static void emitClangAttrTypeArgList(RecordKeeper &Records, raw_ostream &OS) {
   OS << "#if defined(CLANG_ATTR_TYPE_ARG_LIST)\n";
@@ -2149,7 +2176,7 @@
 if (Args.empty())
   continue;
 
-if (Args[0]->getSuperClasses().back().first->getName() != "TypeArgument")
+if (!isTypeArgument(Args[0]))
   continue;
 
 // All these spellings take a single type argument.
@@ -2179,7 +2206,7 @@
   OS << "#endif // CLANG_ATTR_ARG_CONTEXT_LIST\n\n";
 }
 
-static bool isIdentifierArgument(Record *Arg) {
+static bool isIdentifierArgument(const Record *Arg) {
   return !Arg->getSuperClasses().empty() &&
 llvm::StringSwitch(Arg->getSuperClasses().back().first->getName())
 .Case("IdentifierArgument", true)
@@ -2188,7 +2215,7 @@
 .Default(false);
 }
 
-static bool isVariadicIdentifierArgument(Record *Arg) {
+static bool isVariadicIdentifierArgument(const Record *Arg) {
   return !Arg->getSuperClasses().empty() &&
  llvm::StringSwitch(
  Arg->getSuperClasses().back().first->getName())
@@ -2264,6 +2291,26 @@
   OS << "#endif // CLANG_ATTR_THIS_ISA_IDENTIFIER_ARG_LIST\n\n";
 }
 
+static void emitClangAttrAcc

[PATCH] D114439: [Annotation] Allow parameter pack expansions and initializer lists in annotate attribute

[Steffen Larsen via Phabricator via cfe-commits]

steffenlarsen added inline comments.



Comment at: clang/include/clang/Basic/Attr.td:545
+  // Set to true if this attribute accepts parameter pack expansion 
expressions.
+  bit AcceptsExprPack = 0;
   // Lists language options, one of which is required to be true for the

aaron.ballman wrote:
> You should probably add a release note about this new feature.
I have added a release note about this under "Internal API Changes". Let me 
know if you think the wording needs changes.



Comment at: clang/include/clang/Basic/Attr.td:789
   let PragmaAttributeSupport = 1;
+  let AcceptsExprPack = 1;
   let Documentation = [Undocumented];

aaron.ballman wrote:
> You should definitely add a release note about this new behavior for annotate.
I have added a release note about this under "Attribute Changes in Clang". As 
with the other note, let me know if wording needs changes.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:8153
+
+  bool AttrHasVariadicArg = AL.hasVariadicArg();
+  unsigned AttrNumArgs = AL.getNumArgMembers();

erichkeane wrote:
> steffenlarsen wrote:
> > erichkeane wrote:
> > > This still doesn't work if the VariadicExprArgument isn't last, right?  
> > > Do we ensure that is the case in clang-attr-emitter?
> > Good point. I don't think there's a check as there are select few 
> > attributes that use multiple variadic (`OMPDeclareSimdDecl` and 
> > `OMPDeclareVariant` are the only ones, I think.)
> > 
> > Since I don't think it's safe to check for all, should I make a check 
> > similar to the one for type/identifier arguments in attributes marked 
> > `AcceptsExprPack`? Would that suffice?
> I'm fine rejecting a case that has anything besides expression-arguments(and 
> ones create-able from expression arguments) and 
> limited-to-only-1-must-be-last variadic-expr-list in ClangAttrEmitter.
> 
> I believe we discussed that at one point, but I didn't see it here.
I have added a check to ClangAttrEmitter next to the check ensuring that the 
relevant attributes don't have identifier or type arguments.


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

https://reviews.llvm.org/D114439

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114439: [Annotation] Allow parameter pack expansions and initializer lists in annotate attribute

[Steffen Larsen via Phabricator via cfe-commits]

steffenlarsen updated this revision to Diff 406094.
steffenlarsen added a comment.

Added missing `isVariadicExprArgument` function.


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

https://reviews.llvm.org/D114439

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/include/clang/Parse/Parser.h
  clang/include/clang/Sema/ParsedAttr.h
  clang/include/clang/Sema/Sema.h
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Parse/ParseExpr.cpp
  clang/lib/Sema/ParsedAttr.cpp
  clang/lib/Sema/SemaAttr.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/Parser/cxx0x-attributes.cpp
  clang/test/Sema/annotate.c
  clang/test/SemaCXX/attr-annotate.cpp
  clang/test/SemaTemplate/attributes.cpp
  clang/utils/TableGen/ClangAttrEmitter.cpp

Index: clang/utils/TableGen/ClangAttrEmitter.cpp
===
--- clang/utils/TableGen/ClangAttrEmitter.cpp
+++ clang/utils/TableGen/ClangAttrEmitter.cpp
@@ -202,9 +202,9 @@
 bool Fake;
 
   public:
-Argument(const Record &Arg, StringRef Attr)
-: lowerName(std::string(Arg.getValueAsString("Name"))),
-  upperName(lowerName), attrName(Attr), isOpt(false), Fake(false) {
+Argument(StringRef Arg, StringRef Attr)
+: lowerName(std::string(Arg)), upperName(lowerName), attrName(Attr),
+  isOpt(false), Fake(false) {
   if (!lowerName.empty()) {
 lowerName[0] = std::tolower(lowerName[0]);
 upperName[0] = std::toupper(upperName[0]);
@@ -215,6 +215,8 @@
   if (lowerName == "interface")
 lowerName = "interface_";
 }
+Argument(const Record &Arg, StringRef Attr)
+: Argument(Arg.getValueAsString("Name"), Attr) {}
 virtual ~Argument() = default;
 
 StringRef getLowerName() const { return lowerName; }
@@ -666,6 +668,11 @@
   ArgName(getLowerName().str() + "_"), ArgSizeName(ArgName + "Size"),
   RangeName(std::string(getLowerName())) {}
 
+VariadicArgument(StringRef Arg, StringRef Attr, std::string T)
+: Argument(Arg, Attr), Type(std::move(T)),
+  ArgName(getLowerName().str() + "_"), ArgSizeName(ArgName + "Size"),
+  RangeName(std::string(getLowerName())) {}
+
 const std::string &getType() const { return Type; }
 const std::string &getArgName() const { return ArgName; }
 const std::string &getArgSizeName() const { return ArgSizeName; }
@@ -688,6 +695,18 @@
  << "); }\n";
 }
 
+void writeSetter(raw_ostream &OS) const {
+  OS << "  void set" << getUpperName() << "(ASTContext &Ctx, ";
+  writeCtorParameters(OS);
+  OS << ") {\n";
+  OS << "" << ArgSizeName << " = " << getUpperName() << "Size;\n";
+  OS << "" << ArgName << " = new (Ctx, 16) " << getType() << "["
+ << ArgSizeName << "];\n";
+  OS << "  ";
+  writeCtorBody(OS);
+  OS << "  }\n";
+}
+
 void writeCloneArgs(raw_ostream &OS) const override {
   OS << ArgName << ", " << ArgSizeName;
 }
@@ -1169,6 +1188,9 @@
   : VariadicArgument(Arg, Attr, "Expr *")
 {}
 
+VariadicExprArgument(StringRef ArgName, StringRef Attr)
+: VariadicArgument(ArgName, Attr, "Expr *") {}
+
 void writeASTVisitorTraversal(raw_ostream &OS) const override {
   OS << "  {\n";
   OS << "" << getType() << " *I = A->" << getLowerName()
@@ -2138,6 +2160,11 @@
   }
 }
 
+static bool isTypeArgument(const Record *Arg) {
+  return !Arg->getSuperClasses().empty() &&
+ Arg->getSuperClasses().back().first->getName() == "TypeArgument";
+}
+
 /// Emits the first-argument-is-type property for attributes.
 static void emitClangAttrTypeArgList(RecordKeeper &Records, raw_ostream &OS) {
   OS << "#if defined(CLANG_ATTR_TYPE_ARG_LIST)\n";
@@ -2149,7 +2176,7 @@
 if (Args.empty())
   continue;
 
-if (Args[0]->getSuperClasses().back().first->getName() != "TypeArgument")
+if (!isTypeArgument(Args[0]))
   continue;
 
 // All these spellings take a single type argument.
@@ -2179,7 +2206,7 @@
   OS << "#endif // CLANG_ATTR_ARG_CONTEXT_LIST\n\n";
 }
 
-static bool isIdentifierArgument(Record *Arg) {
+static bool isIdentifierArgument(const Record *Arg) {
   return !Arg->getSuperClasses().empty() &&
 llvm::StringSwitch(Arg->getSuperClasses().back().first->getName())
 .Case("IdentifierArgument", true)
@@ -2188,7 +2215,7 @@
 .Default(false);
 }
 
-static bool isVariadicIdentifierArgument(Record *Arg) {
+static bool isVariadicIdentifierArgument(const Record *Arg) {
   return !Arg->getSuperClasses().empty() &&
  llvm::StringSwitch(
  Arg->getSuperClasses().back().first->getName())
@@ -2197,6 +2224,14 @@
  .Default(false);
 }
 
+static bool isVariadicExprArgument(const Record *Arg) {
+  return !Arg->getSuperClasses().empty() &&
+ llvm::StringSwitch(
+ Arg->getSupe

[PATCH] D114439: [Annotation] Allow parameter pack expansions and initializer lists in annotate attribute

[Steffen Larsen via Phabricator via cfe-commits]

steffenlarsen updated this revision to Diff 406373.
steffenlarsen added a comment.

Added a check to prevent duplicate "empty" constructors, i.e. if there are only 
optional arguments such as a single variadic argument.


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

https://reviews.llvm.org/D114439

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/include/clang/Parse/Parser.h
  clang/include/clang/Sema/ParsedAttr.h
  clang/include/clang/Sema/Sema.h
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Parse/ParseExpr.cpp
  clang/lib/Sema/ParsedAttr.cpp
  clang/lib/Sema/SemaAttr.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/Parser/cxx0x-attributes.cpp
  clang/test/Sema/annotate.c
  clang/test/SemaCXX/attr-annotate.cpp
  clang/test/SemaTemplate/attributes.cpp
  clang/utils/TableGen/ClangAttrEmitter.cpp

Index: clang/utils/TableGen/ClangAttrEmitter.cpp
===
--- clang/utils/TableGen/ClangAttrEmitter.cpp
+++ clang/utils/TableGen/ClangAttrEmitter.cpp
@@ -202,9 +202,9 @@
 bool Fake;
 
   public:
-Argument(const Record &Arg, StringRef Attr)
-: lowerName(std::string(Arg.getValueAsString("Name"))),
-  upperName(lowerName), attrName(Attr), isOpt(false), Fake(false) {
+Argument(StringRef Arg, StringRef Attr)
+: lowerName(std::string(Arg)), upperName(lowerName), attrName(Attr),
+  isOpt(false), Fake(false) {
   if (!lowerName.empty()) {
 lowerName[0] = std::tolower(lowerName[0]);
 upperName[0] = std::toupper(upperName[0]);
@@ -215,6 +215,8 @@
   if (lowerName == "interface")
 lowerName = "interface_";
 }
+Argument(const Record &Arg, StringRef Attr)
+: Argument(Arg.getValueAsString("Name"), Attr) {}
 virtual ~Argument() = default;
 
 StringRef getLowerName() const { return lowerName; }
@@ -666,6 +668,11 @@
   ArgName(getLowerName().str() + "_"), ArgSizeName(ArgName + "Size"),
   RangeName(std::string(getLowerName())) {}
 
+VariadicArgument(StringRef Arg, StringRef Attr, std::string T)
+: Argument(Arg, Attr), Type(std::move(T)),
+  ArgName(getLowerName().str() + "_"), ArgSizeName(ArgName + "Size"),
+  RangeName(std::string(getLowerName())) {}
+
 const std::string &getType() const { return Type; }
 const std::string &getArgName() const { return ArgName; }
 const std::string &getArgSizeName() const { return ArgSizeName; }
@@ -688,6 +695,18 @@
  << "); }\n";
 }
 
+void writeSetter(raw_ostream &OS) const {
+  OS << "  void set" << getUpperName() << "(ASTContext &Ctx, ";
+  writeCtorParameters(OS);
+  OS << ") {\n";
+  OS << "" << ArgSizeName << " = " << getUpperName() << "Size;\n";
+  OS << "" << ArgName << " = new (Ctx, 16) " << getType() << "["
+ << ArgSizeName << "];\n";
+  OS << "  ";
+  writeCtorBody(OS);
+  OS << "  }\n";
+}
+
 void writeCloneArgs(raw_ostream &OS) const override {
   OS << ArgName << ", " << ArgSizeName;
 }
@@ -1169,6 +1188,9 @@
   : VariadicArgument(Arg, Attr, "Expr *")
 {}
 
+VariadicExprArgument(StringRef ArgName, StringRef Attr)
+: VariadicArgument(ArgName, Attr, "Expr *") {}
+
 void writeASTVisitorTraversal(raw_ostream &OS) const override {
   OS << "  {\n";
   OS << "" << getType() << " *I = A->" << getLowerName()
@@ -2138,6 +2160,11 @@
   }
 }
 
+static bool isTypeArgument(const Record *Arg) {
+  return !Arg->getSuperClasses().empty() &&
+ Arg->getSuperClasses().back().first->getName() == "TypeArgument";
+}
+
 /// Emits the first-argument-is-type property for attributes.
 static void emitClangAttrTypeArgList(RecordKeeper &Records, raw_ostream &OS) {
   OS << "#if defined(CLANG_ATTR_TYPE_ARG_LIST)\n";
@@ -2149,7 +2176,7 @@
 if (Args.empty())
   continue;
 
-if (Args[0]->getSuperClasses().back().first->getName() != "TypeArgument")
+if (!isTypeArgument(Args[0]))
   continue;
 
 // All these spellings take a single type argument.
@@ -2179,7 +2206,7 @@
   OS << "#endif // CLANG_ATTR_ARG_CONTEXT_LIST\n\n";
 }
 
-static bool isIdentifierArgument(Record *Arg) {
+static bool isIdentifierArgument(const Record *Arg) {
   return !Arg->getSuperClasses().empty() &&
 llvm::StringSwitch(Arg->getSuperClasses().back().first->getName())
 .Case("IdentifierArgument", true)
@@ -2188,7 +2215,7 @@
 .Default(false);
 }
 
-static bool isVariadicIdentifierArgument(Record *Arg) {
+static bool isVariadicIdentifierArgument(const Record *Arg) {
   return !Arg->getSuperClasses().empty() &&
  llvm::StringSwitch(
  Arg->getSuperClasses().back().first->getName())
@@ -2197,6 +2224,14 @@
  .Default(false);
 }
 
+static bool isVariadicExprArgument(const Record *Arg) {
+  return !Arg

[PATCH] D114439: [Annotation] Allow parameter pack expansions and initializer lists in annotate attribute

[Steffen Larsen via Phabricator via cfe-commits]

steffenlarsen updated this revision to Diff 406468.
steffenlarsen added a comment.

Recent changes:

- Replaces `isExprArg` with `isArgMemberExprHolder` which returns true if the 
"argument member" can hold one or more expressions.
- Renames `checkStringLiteralExpr` to `checkASCIIStringLiteralExpr` to better 
convey the intended semantics.
- Reverts changes to `HandleAnnotateAttr` and removes `HandleAnnotationAttr`, 
moving final-args checks of expressions into 
`instantiateDependentAnnotationAttr`.
- Removes nonsensical comment.


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

https://reviews.llvm.org/D114439

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/include/clang/Parse/Parser.h
  clang/include/clang/Sema/ParsedAttr.h
  clang/include/clang/Sema/Sema.h
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Parse/ParseExpr.cpp
  clang/lib/Sema/ParsedAttr.cpp
  clang/lib/Sema/SemaAttr.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/Parser/cxx0x-attributes.cpp
  clang/test/Sema/annotate.c
  clang/test/SemaCXX/attr-annotate.cpp
  clang/test/SemaTemplate/attributes.cpp
  clang/utils/TableGen/ClangAttrEmitter.cpp

Index: clang/utils/TableGen/ClangAttrEmitter.cpp
===
--- clang/utils/TableGen/ClangAttrEmitter.cpp
+++ clang/utils/TableGen/ClangAttrEmitter.cpp
@@ -202,9 +202,9 @@
 bool Fake;
 
   public:
-Argument(const Record &Arg, StringRef Attr)
-: lowerName(std::string(Arg.getValueAsString("Name"))),
-  upperName(lowerName), attrName(Attr), isOpt(false), Fake(false) {
+Argument(StringRef Arg, StringRef Attr)
+: lowerName(std::string(Arg)), upperName(lowerName), attrName(Attr),
+  isOpt(false), Fake(false) {
   if (!lowerName.empty()) {
 lowerName[0] = std::tolower(lowerName[0]);
 upperName[0] = std::toupper(upperName[0]);
@@ -215,6 +215,8 @@
   if (lowerName == "interface")
 lowerName = "interface_";
 }
+Argument(const Record &Arg, StringRef Attr)
+: Argument(Arg.getValueAsString("Name"), Attr) {}
 virtual ~Argument() = default;
 
 StringRef getLowerName() const { return lowerName; }
@@ -666,6 +668,11 @@
   ArgName(getLowerName().str() + "_"), ArgSizeName(ArgName + "Size"),
   RangeName(std::string(getLowerName())) {}
 
+VariadicArgument(StringRef Arg, StringRef Attr, std::string T)
+: Argument(Arg, Attr), Type(std::move(T)),
+  ArgName(getLowerName().str() + "_"), ArgSizeName(ArgName + "Size"),
+  RangeName(std::string(getLowerName())) {}
+
 const std::string &getType() const { return Type; }
 const std::string &getArgName() const { return ArgName; }
 const std::string &getArgSizeName() const { return ArgSizeName; }
@@ -688,6 +695,18 @@
  << "); }\n";
 }
 
+void writeSetter(raw_ostream &OS) const {
+  OS << "  void set" << getUpperName() << "(ASTContext &Ctx, ";
+  writeCtorParameters(OS);
+  OS << ") {\n";
+  OS << "" << ArgSizeName << " = " << getUpperName() << "Size;\n";
+  OS << "" << ArgName << " = new (Ctx, 16) " << getType() << "["
+ << ArgSizeName << "];\n";
+  OS << "  ";
+  writeCtorBody(OS);
+  OS << "  }\n";
+}
+
 void writeCloneArgs(raw_ostream &OS) const override {
   OS << ArgName << ", " << ArgSizeName;
 }
@@ -1169,6 +1188,9 @@
   : VariadicArgument(Arg, Attr, "Expr *")
 {}
 
+VariadicExprArgument(StringRef ArgName, StringRef Attr)
+: VariadicArgument(ArgName, Attr, "Expr *") {}
+
 void writeASTVisitorTraversal(raw_ostream &OS) const override {
   OS << "  {\n";
   OS << "" << getType() << " *I = A->" << getLowerName()
@@ -2138,6 +2160,11 @@
   }
 }
 
+static bool isTypeArgument(const Record *Arg) {
+  return !Arg->getSuperClasses().empty() &&
+ Arg->getSuperClasses().back().first->getName() == "TypeArgument";
+}
+
 /// Emits the first-argument-is-type property for attributes.
 static void emitClangAttrTypeArgList(RecordKeeper &Records, raw_ostream &OS) {
   OS << "#if defined(CLANG_ATTR_TYPE_ARG_LIST)\n";
@@ -2149,7 +2176,7 @@
 if (Args.empty())
   continue;
 
-if (Args[0]->getSuperClasses().back().first->getName() != "TypeArgument")
+if (!isTypeArgument(Args[0]))
   continue;
 
 // All these spellings take a single type argument.
@@ -2179,7 +2206,7 @@
   OS << "#endif // CLANG_ATTR_ARG_CONTEXT_LIST\n\n";
 }
 
-static bool isIdentifierArgument(Record *Arg) {
+static bool isIdentifierArgument(const Record *Arg) {
   return !Arg->getSuperClasses().empty() &&
 llvm::StringSwitch(Arg->getSuperClasses().back().first->getName())
 .Case("IdentifierArgument", true)
@@ -2188,7 +2215,7 @@
 .Default(false);
 }
 
-static bool isVariadicIdentifierArgument(Record *Arg) {
+static bool i

[PATCH] D114439: [Annotation] Allow parameter pack expansions and initializer lists in annotate attribute

[Steffen Larsen via Phabricator via cfe-commits]

steffenlarsen added inline comments.



Comment at: clang/include/clang/Sema/ParsedAttr.h:113
   }
+  // Check if argument at index I is an expression argument
+  virtual bool isExprArg(size_t N) const { return false; }

aaron.ballman wrote:
> I don't think this API is needed. `isArgExpr()` already exists on the 
> interface and I'm not certain how this differs. (If this API is needed, we 
> need a far better name for it because I'd have no idea how to distinguish 
> `isExprArg()` and `isArgExpr()`.)
There is an important distinction between the two, specifically that 
`isArgExpr` checks if the parsed argument at the index is an expression, while 
`isExprArg` checks if the specified argument in the attribute is an 
"ExprArgument", i.e. one checks post-parsing the other reflects on the 
attribute's definition.

That said, I agree that with that naming there's little distinction between the 
two. I have therefore removed `isExprArg` and replaced it with 
`isArgMemberExprHolder` which is true if the "argument member" (referring to 
the argument defined in Attr.td) is able to hold an expression (i.e. it is 
either a `ExprArgument` or a `VaridicExprArgument`.) Functionally there is 
little change to it, as the only place we use the check is after checking if 
the expression is variadic, but it fit better with the actual intent of the 
check.



Comment at: clang/include/clang/Sema/Sema.h:4383-4385
+  bool checkStringLiteralExpr(const AttributeCommonInfo &CI, const Expr *E,
+  StringRef &Str,
+  SourceLocation *ArgLocation = nullptr);

aaron.ballman wrote:
> This name makes it sound like far more general of an API than it is. This 
> function is specific to checking string literal arguments for an attribute. 
> But then why does `checkStringLiteralArgumentAttr()` not suffice? It does the 
> same thing, but with more bells and whistles.
The problem is that `checkStringLiteralArgumentAttr()` requires the arguments 
to be extracted from the parsed attribute, which we do not have after template 
instantiation, so the split was for generality. However, as you mentioned in 
another comment, the generality is not that needed anymore so 
`handleAnnotateAttr` has been reverted to the old version.

That said, this is still needed for doing post-initalization check if arguments 
have been delayed, so I have renamed it to `checkASCIIStringLiteralExpr` which 
hopefully conveys the intent a little better?



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4185
   StringRef Str;
-  if (!S.checkStringLiteralArgumentAttr(AL, 0, Str))
+  if (!checkStringLiteralExpr(CI, AllArgs[0], Str))
 return;

aaron.ballman wrote:
> This is a slight loss of functionality. We used to recommend identifiers be 
> converted to string literals under the old interface. I don't think this 
> refactoring is necessary any longer, right?
You're absolutely right. There is actually no reason for this refactoring 
anymore, except for maybe some minor common code between this and delayed 
argument instantiation, but it is so little now that if we want to keep the 
hint (which I agree we should) then there's little reason not to revert the 
changes to `handleAnnotateAttr`. So that's what I've done! Similarly, 
`handleAnnotateAttr` is no longer needed so it has been removed and the excess 
logic has been moved to `instantiateDependentAnnotationAttr`. Thanks for 
pointing this out!



Comment at: clang/utils/TableGen/ClangAttrEmitter.cpp:2312
+
+// All these spellings take are parsed unevaluated.
+forEachUniqueSpelling(Attr, [&](const FlattenedSpelling &S) {

aaron.ballman wrote:
> The comment doesn't seem grammatical and may need some expounding.
I don't think a comment makes much sense here anyway, so I've removed it. This 
seems like fairly common practice for these kind of functions. If you disagree 
I can reintroduce it and expand on it.


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

https://reviews.llvm.org/D114439

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114439: [Annotation] Allow parameter pack expansions and initializer lists in annotate attribute

[Steffen Larsen via Phabricator via cfe-commits]

steffenlarsen updated this revision to Diff 406868.
steffenlarsen added a comment.

Recent changes:

- Renamed `isArgMemberExprHolder` to `isParamExpr` and added the comment 
suggested by @aaron.ballman .
- Made `checkASCIIStringLiteralExpr` an overload of 
`checkStringLiteralArgumentAttr`.


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

https://reviews.llvm.org/D114439

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/include/clang/Parse/Parser.h
  clang/include/clang/Sema/ParsedAttr.h
  clang/include/clang/Sema/Sema.h
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Parse/ParseExpr.cpp
  clang/lib/Sema/ParsedAttr.cpp
  clang/lib/Sema/SemaAttr.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/Parser/cxx0x-attributes.cpp
  clang/test/Sema/annotate.c
  clang/test/SemaCXX/attr-annotate.cpp
  clang/test/SemaTemplate/attributes.cpp
  clang/utils/TableGen/ClangAttrEmitter.cpp

Index: clang/utils/TableGen/ClangAttrEmitter.cpp
===
--- clang/utils/TableGen/ClangAttrEmitter.cpp
+++ clang/utils/TableGen/ClangAttrEmitter.cpp
@@ -202,9 +202,9 @@
 bool Fake;
 
   public:
-Argument(const Record &Arg, StringRef Attr)
-: lowerName(std::string(Arg.getValueAsString("Name"))),
-  upperName(lowerName), attrName(Attr), isOpt(false), Fake(false) {
+Argument(StringRef Arg, StringRef Attr)
+: lowerName(std::string(Arg)), upperName(lowerName), attrName(Attr),
+  isOpt(false), Fake(false) {
   if (!lowerName.empty()) {
 lowerName[0] = std::tolower(lowerName[0]);
 upperName[0] = std::toupper(upperName[0]);
@@ -215,6 +215,8 @@
   if (lowerName == "interface")
 lowerName = "interface_";
 }
+Argument(const Record &Arg, StringRef Attr)
+: Argument(Arg.getValueAsString("Name"), Attr) {}
 virtual ~Argument() = default;
 
 StringRef getLowerName() const { return lowerName; }
@@ -666,6 +668,11 @@
   ArgName(getLowerName().str() + "_"), ArgSizeName(ArgName + "Size"),
   RangeName(std::string(getLowerName())) {}
 
+VariadicArgument(StringRef Arg, StringRef Attr, std::string T)
+: Argument(Arg, Attr), Type(std::move(T)),
+  ArgName(getLowerName().str() + "_"), ArgSizeName(ArgName + "Size"),
+  RangeName(std::string(getLowerName())) {}
+
 const std::string &getType() const { return Type; }
 const std::string &getArgName() const { return ArgName; }
 const std::string &getArgSizeName() const { return ArgSizeName; }
@@ -688,6 +695,18 @@
  << "); }\n";
 }
 
+void writeSetter(raw_ostream &OS) const {
+  OS << "  void set" << getUpperName() << "(ASTContext &Ctx, ";
+  writeCtorParameters(OS);
+  OS << ") {\n";
+  OS << "" << ArgSizeName << " = " << getUpperName() << "Size;\n";
+  OS << "" << ArgName << " = new (Ctx, 16) " << getType() << "["
+ << ArgSizeName << "];\n";
+  OS << "  ";
+  writeCtorBody(OS);
+  OS << "  }\n";
+}
+
 void writeCloneArgs(raw_ostream &OS) const override {
   OS << ArgName << ", " << ArgSizeName;
 }
@@ -1169,6 +1188,9 @@
   : VariadicArgument(Arg, Attr, "Expr *")
 {}
 
+VariadicExprArgument(StringRef ArgName, StringRef Attr)
+: VariadicArgument(ArgName, Attr, "Expr *") {}
+
 void writeASTVisitorTraversal(raw_ostream &OS) const override {
   OS << "  {\n";
   OS << "" << getType() << " *I = A->" << getLowerName()
@@ -2138,6 +2160,11 @@
   }
 }
 
+static bool isTypeArgument(const Record *Arg) {
+  return !Arg->getSuperClasses().empty() &&
+ Arg->getSuperClasses().back().first->getName() == "TypeArgument";
+}
+
 /// Emits the first-argument-is-type property for attributes.
 static void emitClangAttrTypeArgList(RecordKeeper &Records, raw_ostream &OS) {
   OS << "#if defined(CLANG_ATTR_TYPE_ARG_LIST)\n";
@@ -2149,7 +2176,7 @@
 if (Args.empty())
   continue;
 
-if (Args[0]->getSuperClasses().back().first->getName() != "TypeArgument")
+if (!isTypeArgument(Args[0]))
   continue;
 
 // All these spellings take a single type argument.
@@ -2179,7 +2206,7 @@
   OS << "#endif // CLANG_ATTR_ARG_CONTEXT_LIST\n\n";
 }
 
-static bool isIdentifierArgument(Record *Arg) {
+static bool isIdentifierArgument(const Record *Arg) {
   return !Arg->getSuperClasses().empty() &&
 llvm::StringSwitch(Arg->getSuperClasses().back().first->getName())
 .Case("IdentifierArgument", true)
@@ -2188,7 +2215,7 @@
 .Default(false);
 }
 
-static bool isVariadicIdentifierArgument(Record *Arg) {
+static bool isVariadicIdentifierArgument(const Record *Arg) {
   return !Arg->getSuperClasses().empty() &&
  llvm::StringSwitch(
  Arg->getSuperClasses().back().first->getName())
@@ -2197,6 +2224,14 @@
  .Default(false);
 }
 

[PATCH] D114439: [Annotation] Allow parameter pack expansions and initializer lists in annotate attribute

[Steffen Larsen via Phabricator via cfe-commits]

steffenlarsen added a comment.

Thanks, @aaron.ballman ! I have applied the new suggestions.

> I presume you don't have commit privileges @steffenlarsen? If that's 
> accurate, what name and email address would you like me to use for patch 
> attribution after you've fixed the last few bits?

That is correct. Name is "Steffen Larsen" and email is 
"steffen.lar...@intel.com". Thanks a ton. 😄




Comment at: clang/include/clang/Sema/ParsedAttr.h:113
   }
+  // Check if argument at index I is an expression argument
+  virtual bool isExprArg(size_t N) const { return false; }

aaron.ballman wrote:
> steffenlarsen wrote:
> > aaron.ballman wrote:
> > > I don't think this API is needed. `isArgExpr()` already exists on the 
> > > interface and I'm not certain how this differs. (If this API is needed, 
> > > we need a far better name for it because I'd have no idea how to 
> > > distinguish `isExprArg()` and `isArgExpr()`.)
> > There is an important distinction between the two, specifically that 
> > `isArgExpr` checks if the parsed argument at the index is an expression, 
> > while `isExprArg` checks if the specified argument in the attribute is an 
> > "ExprArgument", i.e. one checks post-parsing the other reflects on the 
> > attribute's definition.
> > 
> > That said, I agree that with that naming there's little distinction between 
> > the two. I have therefore removed `isExprArg` and replaced it with 
> > `isArgMemberExprHolder` which is true if the "argument member" (referring 
> > to the argument defined in Attr.td) is able to hold an expression (i.e. it 
> > is either a `ExprArgument` or a `VaridicExprArgument`.) Functionally there 
> > is little change to it, as the only place we use the check is after 
> > checking if the expression is variadic, but it fit better with the actual 
> > intent of the check.
> Thanks for the clarification!
> 
> I think a better way to express this is by naming it `isParamExpr()` and 
> adding comments along the lines of:
> 
> ```
> /// Returns true if the specified parameter index for this attribute in 
> Attr.td is an ExprArgument
> /// or VariadicExprArgument, or a subclass thereof; returns false otherwise.
> ```
> (Note, the local style here is to use `///` instead of `//`, so I'm matching 
> that as well. You probably need to flow the comments to 80 cols as well.)
`isParamExpr` is fine by me! I have also added the suggested comment. Thanks! 
(Also, good eye on the `//` vs `///`)



Comment at: clang/include/clang/Sema/Sema.h:4383-4385
+  bool checkStringLiteralExpr(const AttributeCommonInfo &CI, const Expr *E,
+  StringRef &Str,
+  SourceLocation *ArgLocation = nullptr);

aaron.ballman wrote:
> steffenlarsen wrote:
> > aaron.ballman wrote:
> > > This name makes it sound like far more general of an API than it is. This 
> > > function is specific to checking string literal arguments for an 
> > > attribute. But then why does `checkStringLiteralArgumentAttr()` not 
> > > suffice? It does the same thing, but with more bells and whistles.
> > The problem is that `checkStringLiteralArgumentAttr()` requires the 
> > arguments to be extracted from the parsed attribute, which we do not have 
> > after template instantiation, so the split was for generality. However, as 
> > you mentioned in another comment, the generality is not that needed anymore 
> > so `handleAnnotateAttr` has been reverted to the old version.
> > 
> > That said, this is still needed for doing post-initalization check if 
> > arguments have been delayed, so I have renamed it to 
> > `checkASCIIStringLiteralExpr` which hopefully conveys the intent a little 
> > better?
> The new name is still a problem -- it sounds like a generic interface, but 
> the diagnostic it emits is specific to attributes. That's going to be a 
> maintenance issue.
> 
> I would make this an overload of `checkStringLiteralArgumentAttr()` instead 
> of giving it a new name.
Overloading `checkStringLiteralArgumentAttr` is a good shout. I have done that.


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

https://reviews.llvm.org/D114439

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100394: [Clang][NVPTX] Add NVPTX intrinsics and builtins for CUDA PTX cp.async instructions

[Steffen Larsen via Phabricator via cfe-commits]

steffenlarsen added a comment.

In D100394#3466316 , @nirvedhmeshram 
wrote:

> Hello, I was interested in using `llvm.nvvm.cp.async.cg.shared.global.8` and 
> `llvm.nvvm.cp.async.cg.shared.global.4` and was wondering if there is some 
> fundamental reason they were not added here. I only see the ca variants for 
> these.

Hi @nirvedhmeshram! According to the PTX ISA 

 there is only a 16 variant of `cp.async.cg.shared.global`. That said, they 
have an example further down using 8 with it, so it seems there's either a 
problem in the Syntax subsection or the examples. Either way, that is the 
explanation as to why it was not added with this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100394

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100124: [Clang][NVPTX] Add NVPTX intrinsics and builtins for CUDA PTX redux.sync instructions

[Steffen Larsen via Phabricator via cfe-commits]

steffenlarsen added a comment.

@tra Thanks a ton for the review! This is my first LLVM patch so I only know as 
much as the Code Review documentation tells me. Is there a process for chasing 
up additional reviews?


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

https://reviews.llvm.org/D100124

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100124: [Clang][NVPTX] Add NVPTX intrinsics and builtins for CUDA PTX redux.sync instructions

[Steffen Larsen via Phabricator via cfe-commits]

steffenlarsen added a comment.

In D100124#2757306 , @tra wrote:

> Do you have ability to commit to LLVM? If not, I can land the patch on your 
> behalf.

Not to my knowledge, so please do. Thanks again!

https://reviews.llvm.org/D100394 is from my colleagues and it also looks ready. 
Would you mind landing that one as well? 😄


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

https://reviews.llvm.org/D100124

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D104847: [Clang][NVPTX] Add NVPTX intrinsics and builtins for CUDA PTX 6.5 and 7.0 WMMA and MMA instructions

[Steffen Larsen via Phabricator via cfe-commits]

steffenlarsen created this revision.
steffenlarsen added a reviewer: tra.
Herald added subscribers: hiraditya, yaxunl, jholewinski.
steffenlarsen requested review of this revision.
Herald added subscribers: llvm-commits, cfe-commits, jdoerfert.
Herald added projects: clang, LLVM.

Adds NVPTX builtins and intrinsics for the CUDA PTX `wmma.load`, `wmma.store`, 
`wmma.mma`, and `mma` instructions added in PTX 6.5 and 7.0.

PTX ISA description of

- `wmma.load`: 
https://docs.nvidia.com/cuda/parallel-thread-execution/index.html#warp-level-matrix-instructions-wmma-ld
- `wmma.store`: 
https://docs.nvidia.com/cuda/parallel-thread-execution/index.html#warp-level-matrix-instructions-wmma-st
- `wmma.mma`: 
https://docs.nvidia.com/cuda/parallel-thread-execution/index.html#warp-level-matrix-instructions-wmma-mma
- `mma`: 
https://docs.nvidia.com/cuda/parallel-thread-execution/index.html#warp-level-matrix-instructions-mma

Overview of `wmma.mma` and `mma` matrix shape/type combinations added with 
specific PTX versions: 
https://docs.nvidia.com/cuda/parallel-thread-execution/index.html#warp-level-matrix-shape

Authored-by: Steffen Larsen 
Co-Authored-by: Stuart Adams 


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D104847

Files:
  clang/include/clang/Basic/BuiltinsNVPTX.def
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/test/CodeGen/builtins-nvptx-mma.cu
  clang/test/CodeGen/builtins-nvptx-mma.py
  llvm/include/llvm/IR/IntrinsicsNVVM.td
  llvm/lib/Target/NVPTX/NVPTXISelLowering.cpp
  llvm/lib/Target/NVPTX/NVPTXInstrInfo.td
  llvm/lib/Target/NVPTX/NVPTXIntrinsics.td
  llvm/test/CodeGen/NVPTX/lit.local.cfg
  llvm/test/CodeGen/NVPTX/wmma.py

Index: llvm/test/CodeGen/NVPTX/wmma.py
===
--- llvm/test/CodeGen/NVPTX/wmma.py
+++ llvm/test/CodeGen/NVPTX/wmma.py
@@ -6,7 +6,7 @@
 # RUN: FileCheck %t-ptx60-sm_70.ll < %t-ptx60-sm_70.ll \
 # RUN:   --check-prefixes=INTRINSICS,M16N16
 # RUN: FileCheck %t-ptx60-sm_70.ll < %t-ptx60-sm_70.ll \
-# RUN:   --check-prefixes=INTRINSICS,NOEXTGEOM,NOINT,NOSUBINT,NOMMA
+# RUN:   --check-prefixes=INTRINSICS,NOEXTGEOM,NOINT,NOSUBINT,NOMMA,NODOUBLE,NOALTFLOAT
 # RUN: llc < %t-ptx60-sm_70.ll -march=nvptx64 -mcpu=sm_70 -mattr=+ptx60 \
 # RUN:   | FileCheck %t-ptx60-sm_70.ll
 
@@ -15,7 +15,7 @@
 # RUN: FileCheck %t-ptx61-sm_70.ll < %t-ptx61-sm_70.ll \
 # RUN:   --check-prefixes=INTRINSICS,M16N16,EXTGEOM
 # RUN: FileCheck %t-ptx61-sm_70.ll < %t-ptx61-sm_70.ll \
-# RUN:   --check-prefixes=INTRINSICS,NOINT,NOSUBINT,NOMMA
+# RUN:   --check-prefixes=INTRINSICS,NOINT,NOSUBINT,NOMMA,NODOUBLE,NOALTFLOAT
 # RUN: llc < %t-ptx61-sm_70.ll -march=nvptx64 -mcpu=sm_70 -mattr=+ptx61 \
 # RUN:   | FileCheck %t-ptx61-sm_70.ll
 
@@ -24,7 +24,7 @@
 # RUN: FileCheck %t-ptx63-sm_72.ll < %t-ptx63-sm_72.ll \
 # RUN:   --check-prefixes=INTRINSICS,M16N16,EXTGEOM,INT
 # RUN: FileCheck %t-ptx63-sm_72.ll < %t-ptx63-sm_72.ll \
-# RUN:   --check-prefixes=INTRINSICS,NOSUBINT,NOMMA
+# RUN:   --check-prefixes=INTRINSICS,NOSUBINT,NOMMA,NODOUBLE,NOALTFLOAT
 # RUN: llc < %t-ptx63-sm_72.ll -march=nvptx64 -mcpu=sm_72 -mattr=+ptx63 \
 # RUN:   | FileCheck %t-ptx63-sm_72.ll
 
@@ -33,7 +33,7 @@
 # RUN: FileCheck %t-ptx63-sm_75.ll < %t-ptx63-sm_75.ll \
 # RUN:   --check-prefixes=INTRINSICS,M16N16,EXTGEOM,INT,SUBINT
 # RUN: FileCheck %t-ptx63-sm_75.ll < %t-ptx63-sm_75.ll \
-# RUN:   --check-prefixes=INTRINSICS,NOMMA
+# RUN:   --check-prefixes=INTRINSICS,NOMMA,NODOUBLE,NOALTFLOAT
 # RUN: llc < %t-ptx63-sm_75.ll -march=nvptx64 -mcpu=sm_75 -mattr=+ptx63 \
 # RUN:   | FileCheck %t-ptx63-sm_75.ll
 
@@ -42,10 +42,28 @@
 # RUN: FileCheck %t-ptx64-sm_70.ll < %t-ptx64-sm_70.ll \
 # RUN:   --check-prefixes=INTRINSICS,M16N16,EXTGEOM,MMA
 # RUN: FileCheck %t-ptx64-sm_70.ll < %t-ptx64-sm_70.ll \
-# RUN:   --check-prefixes=INTRINSICS,NOINT,NOSUBINT
+# RUN:   --check-prefixes=INTRINSICS,NOINT,NOSUBINT,NODOUBLE,NOALTFLOAT
 # RUN: llc < %t-ptx64-sm_70.ll -march=nvptx64 -mcpu=sm_70 -mattr=+ptx64 \
 # RUN:   | FileCheck %t-ptx64-sm_70.ll
 
+# Check all variants of instructions supported by PTX65 on SM75+
+# RUN: python %s --ptx=65 --gpu-arch=75 > %t-ptx65-sm_75.ll
+# RUN: FileCheck %t-ptx65-sm_75.ll < %t-ptx65-sm_75.ll \
+# RUN:   --check-prefixes=INTRINSICS,M16N16,EXTGEOM,INT,SUBINT,MMA,PTX65MMA
+# RUN: FileCheck %t-ptx65-sm_75.ll < %t-ptx65-sm_75.ll \
+# RUN:   --check-prefixes=INTRINSICS
+# RUN: llc < %t-ptx65-sm_75.ll -march=nvptx64 -mcpu=sm_75 -mattr=+ptx65 \
+# RUN:   | FileCheck %t-ptx65-sm_75.ll
+
+# Check all variants of instructions supported by PTX70 on SM80+
+# RUN: python %s --ptx=70 --gpu-arch=80 > %t-ptx70-sm_80.ll
+# RUN: FileCheck %t-ptx70-sm_80.ll < %t-ptx70-sm_80.ll \
+# RUN:   --check-prefixes=INTRINSICS,M16N16,EXTGEOM,INT,SUBINT,MMA,ALTFLOAT,DOUBLE,PTX65MMA,PTX70MMA
+# RUN: Fi

[PATCH] D107054: [Clang][CUDA] Add descriptors, mappings, and features for missing CUDA and PTX versions

[Steffen Larsen via Phabricator via cfe-commits]

steffenlarsen updated this revision to Diff 362986.
steffenlarsen added a comment.

Made sure the PTX73 and PTX74 macros are popped correctly.


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

https://reviews.llvm.org/D107054

Files:
  clang/include/clang/Basic/BuiltinsNVPTX.def
  clang/include/clang/Basic/Cuda.h
  clang/lib/Basic/Cuda.cpp
  clang/lib/Basic/Targets/NVPTX.cpp
  clang/lib/Driver/ToolChains/Cuda.cpp
  llvm/lib/Target/NVPTX/NVPTX.td
  llvm/lib/Target/NVPTX/NVPTXInstrInfo.td

Index: llvm/lib/Target/NVPTX/NVPTXInstrInfo.td
===
--- llvm/lib/Target/NVPTX/NVPTXInstrInfo.td
+++ llvm/lib/Target/NVPTX/NVPTXInstrInfo.td
@@ -147,6 +147,9 @@
 def hasPTX65 : Predicate<"Subtarget->getPTXVersion() >= 65">;
 def hasPTX70 : Predicate<"Subtarget->getPTXVersion() >= 70">;
 def hasPTX71 : Predicate<"Subtarget->getPTXVersion() >= 71">;
+def hasPTX72 : Predicate<"Subtarget->getPTXVersion() >= 72">;
+def hasPTX73 : Predicate<"Subtarget->getPTXVersion() >= 73">;
+def hasPTX74 : Predicate<"Subtarget->getPTXVersion() >= 74">;
 
 def hasSM30 : Predicate<"Subtarget->getSmVersion() >= 30">;
 def hasSM70 : Predicate<"Subtarget->getSmVersion() >= 70">;
Index: llvm/lib/Target/NVPTX/NVPTX.td
===
--- llvm/lib/Target/NVPTX/NVPTX.td
+++ llvm/lib/Target/NVPTX/NVPTX.td
@@ -89,6 +89,10 @@
  "Use PTX version 7.1">;
 def PTX72 : SubtargetFeature<"ptx72", "PTXVersion", "72",
  "Use PTX version 7.2">;
+def PTX73 : SubtargetFeature<"ptx73", "PTXVersion", "73",
+ "Use PTX version 7.3">;
+def PTX74 : SubtargetFeature<"ptx74", "PTXVersion", "74",
+ "Use PTX version 7.4">;
 
 //===--===//
 // NVPTX supported processors.
Index: clang/lib/Driver/ToolChains/Cuda.cpp
===
--- clang/lib/Driver/ToolChains/Cuda.cpp
+++ clang/lib/Driver/ToolChains/Cuda.cpp
@@ -77,6 +77,12 @@
 return CudaVersion::CUDA_110;
   if (raw_version < 11020)
 return CudaVersion::CUDA_111;
+  if (raw_version < 11030)
+return CudaVersion::CUDA_112;
+  if (raw_version < 11040)
+return CudaVersion::CUDA_113;
+  if (raw_version < 11050)
+return CudaVersion::CUDA_114;
   return CudaVersion::LATEST;
 }
 
@@ -131,7 +137,9 @@
   SmallVector Candidates;
 
   // In decreasing order so we prefer newer versions to older versions.
-  std::initializer_list Versions = {"8.0", "7.5", "7.0"};
+  std::initializer_list Versions = {
+  "11.4", "11.3", "11.2", "11.1", "10.2", "10.1", "10.0",
+  "9.2",  "9.1",  "9.0",  "8.0",  "7.5",  "7.0"};
   auto &FS = D.getVFS();
 
   if (Args.hasArg(clang::driver::options::OPT_cuda_path_EQ)) {
@@ -720,6 +728,8 @@
   case CudaVersion::CUDA_##CUDA_VER:   \
 PtxFeature = "+ptx" #PTX_VER;  \
 break;
+CASE_CUDA_VERSION(114, 74);
+CASE_CUDA_VERSION(113, 73);
 CASE_CUDA_VERSION(112, 72);
 CASE_CUDA_VERSION(111, 71);
 CASE_CUDA_VERSION(110, 70);
Index: clang/lib/Basic/Targets/NVPTX.cpp
===
--- clang/lib/Basic/Targets/NVPTX.cpp
+++ clang/lib/Basic/Targets/NVPTX.cpp
@@ -45,6 +45,8 @@
 if (!Feature.startswith("+ptx"))
   continue;
 PTXVersion = llvm::StringSwitch(Feature)
+ .Case("+ptx74", 74)
+ .Case("+ptx73", 73)
  .Case("+ptx72", 72)
  .Case("+ptx71", 71)
  .Case("+ptx70", 70)
Index: clang/lib/Basic/Cuda.cpp
===
--- clang/lib/Basic/Cuda.cpp
+++ clang/lib/Basic/Cuda.cpp
@@ -36,6 +36,10 @@
 return "11.1";
   case CudaVersion::CUDA_112:
 return "11.2";
+  case CudaVersion::CUDA_113:
+return "11.3";
+  case CudaVersion::CUDA_114:
+return "11.4";
   }
   llvm_unreachable("invalid enum");
 }
@@ -54,6 +58,8 @@
   .Case("11.0", CudaVersion::CUDA_110)
   .Case("11.1", CudaVersion::CUDA_111)
   .Case("11.2", CudaVersion::CUDA_112)
+  .Case("11.3", CudaVersion::CUDA_113)
+  .Case("11.4", CudaVersion::CUDA_114)
   .Default(CudaVersion::UNKNOWN);
 }
 
@@ -227,6 +233,10 @@
 return CudaVersion::CUDA_111;
   case 112:
 return CudaVersion::CUDA_112;
+  case 113:
+return CudaVersion::CUDA_113;
+  case 114:
+return CudaVersion::CUDA_114;
   default:
 return CudaVersion::UNKNOWN;
   }
Index: clang/include/clang/Basic/Cuda.h
===
--- clang/include/clang/Basic/Cuda.h
+++ clang/include/clang/Basic/Cuda.h
@@ -31,7 +31,9 @@
   CUDA_110,
   CUDA_111,
   CUDA_112,
-  LATEST = CUDA_112,
+  CUDA_113,
+  CUDA_114,
+  

[PATCH] D107054: [Clang][CUDA] Add descriptors, mappings, and features for missing CUDA and PTX versions

[Steffen Larsen via Phabricator via cfe-commits]

steffenlarsen marked an inline comment as done.
steffenlarsen added inline comments.



Comment at: clang/include/clang/Basic/BuiltinsNVPTX.def:821
 #pragma pop_macro("PTX71")
 #pragma pop_macro("PTX72")

tra wrote:
> You need to pop new macros here.
My bad! It has been corrected.


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

https://reviews.llvm.org/D107054

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D104847: [Clang][NVPTX] Add NVPTX intrinsics and builtins for CUDA PTX 6.5 and 7.0 WMMA and MMA instructions

[Steffen Larsen via Phabricator via cfe-commits]

steffenlarsen updated this revision to Diff 354487.
steffenlarsen added a comment.

Adjusted for comments and fixed formatting issues.


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

https://reviews.llvm.org/D104847

Files:
  clang/include/clang/Basic/BuiltinsNVPTX.def
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/test/CodeGen/builtins-nvptx-mma.cu
  clang/test/CodeGen/builtins-nvptx-mma.py
  llvm/include/llvm/IR/IntrinsicsNVVM.td
  llvm/lib/Target/NVPTX/NVPTXISelLowering.cpp
  llvm/lib/Target/NVPTX/NVPTXInstrInfo.td
  llvm/lib/Target/NVPTX/NVPTXIntrinsics.td
  llvm/test/CodeGen/NVPTX/lit.local.cfg
  llvm/test/CodeGen/NVPTX/wmma.py

Index: llvm/test/CodeGen/NVPTX/wmma.py
===
--- llvm/test/CodeGen/NVPTX/wmma.py
+++ llvm/test/CodeGen/NVPTX/wmma.py
@@ -6,7 +6,7 @@
 # RUN: FileCheck %t-ptx60-sm_70.ll < %t-ptx60-sm_70.ll \
 # RUN:   --check-prefixes=INTRINSICS,M16N16
 # RUN: FileCheck %t-ptx60-sm_70.ll < %t-ptx60-sm_70.ll \
-# RUN:   --check-prefixes=INTRINSICS,NOEXTGEOM,NOINT,NOSUBINT,NOMMA
+# RUN:   --check-prefixes=INTRINSICS,NOEXTGEOM,NOINT,NOSUBINT,NOMMA,NODOUBLE,NOALTFLOAT
 # RUN: llc < %t-ptx60-sm_70.ll -march=nvptx64 -mcpu=sm_70 -mattr=+ptx60 \
 # RUN:   | FileCheck %t-ptx60-sm_70.ll
 
@@ -15,7 +15,7 @@
 # RUN: FileCheck %t-ptx61-sm_70.ll < %t-ptx61-sm_70.ll \
 # RUN:   --check-prefixes=INTRINSICS,M16N16,EXTGEOM
 # RUN: FileCheck %t-ptx61-sm_70.ll < %t-ptx61-sm_70.ll \
-# RUN:   --check-prefixes=INTRINSICS,NOINT,NOSUBINT,NOMMA
+# RUN:   --check-prefixes=INTRINSICS,NOINT,NOSUBINT,NOMMA,NODOUBLE,NOALTFLOAT
 # RUN: llc < %t-ptx61-sm_70.ll -march=nvptx64 -mcpu=sm_70 -mattr=+ptx61 \
 # RUN:   | FileCheck %t-ptx61-sm_70.ll
 
@@ -24,7 +24,7 @@
 # RUN: FileCheck %t-ptx63-sm_72.ll < %t-ptx63-sm_72.ll \
 # RUN:   --check-prefixes=INTRINSICS,M16N16,EXTGEOM,INT
 # RUN: FileCheck %t-ptx63-sm_72.ll < %t-ptx63-sm_72.ll \
-# RUN:   --check-prefixes=INTRINSICS,NOSUBINT,NOMMA
+# RUN:   --check-prefixes=INTRINSICS,NOSUBINT,NOMMA,NODOUBLE,NOALTFLOAT
 # RUN: llc < %t-ptx63-sm_72.ll -march=nvptx64 -mcpu=sm_72 -mattr=+ptx63 \
 # RUN:   | FileCheck %t-ptx63-sm_72.ll
 
@@ -33,7 +33,7 @@
 # RUN: FileCheck %t-ptx63-sm_75.ll < %t-ptx63-sm_75.ll \
 # RUN:   --check-prefixes=INTRINSICS,M16N16,EXTGEOM,INT,SUBINT
 # RUN: FileCheck %t-ptx63-sm_75.ll < %t-ptx63-sm_75.ll \
-# RUN:   --check-prefixes=INTRINSICS,NOMMA
+# RUN:   --check-prefixes=INTRINSICS,NOMMA,NODOUBLE,NOALTFLOAT
 # RUN: llc < %t-ptx63-sm_75.ll -march=nvptx64 -mcpu=sm_75 -mattr=+ptx63 \
 # RUN:   | FileCheck %t-ptx63-sm_75.ll
 
@@ -42,10 +42,28 @@
 # RUN: FileCheck %t-ptx64-sm_70.ll < %t-ptx64-sm_70.ll \
 # RUN:   --check-prefixes=INTRINSICS,M16N16,EXTGEOM,MMA
 # RUN: FileCheck %t-ptx64-sm_70.ll < %t-ptx64-sm_70.ll \
-# RUN:   --check-prefixes=INTRINSICS,NOINT,NOSUBINT
+# RUN:   --check-prefixes=INTRINSICS,NOINT,NOSUBINT,NODOUBLE,NOALTFLOAT
 # RUN: llc < %t-ptx64-sm_70.ll -march=nvptx64 -mcpu=sm_70 -mattr=+ptx64 \
 # RUN:   | FileCheck %t-ptx64-sm_70.ll
 
+# Check all variants of instructions supported by PTX65 on SM75+
+# RUN: python %s --ptx=65 --gpu-arch=75 > %t-ptx65-sm_75.ll
+# RUN: FileCheck %t-ptx65-sm_75.ll < %t-ptx65-sm_75.ll \
+# RUN:   --check-prefixes=INTRINSICS,M16N16,EXTGEOM,INT,SUBINT,MMA,PTX65MMA
+# RUN: FileCheck %t-ptx65-sm_75.ll < %t-ptx65-sm_75.ll \
+# RUN:   --check-prefixes=INTRINSICS
+# RUN: llc < %t-ptx65-sm_75.ll -march=nvptx64 -mcpu=sm_75 -mattr=+ptx65 \
+# RUN:   | FileCheck %t-ptx65-sm_75.ll
+
+# Check all variants of instructions supported by PTX70 on SM80+
+# RUN: python %s --ptx=70 --gpu-arch=80 > %t-ptx70-sm_80.ll
+# RUN: FileCheck %t-ptx70-sm_80.ll < %t-ptx70-sm_80.ll \
+# RUN:   --check-prefixes=INTRINSICS,M16N16,EXTGEOM,INT,SUBINT,MMA,ALTFLOAT,DOUBLE,PTX65MMA,PTX70MMA
+# RUN: FileCheck %t-ptx70-sm_80.ll < %t-ptx70-sm_80.ll \
+# RUN:   --check-prefixes=INTRINSICS
+# RUN: llc < %t-ptx70-sm_80.ll -march=nvptx64 -mcpu=sm_80 -mattr=+ptx70 \
+# RUN:   | FileCheck %t-ptx70-sm_80.ll
+
 from __future__ import print_function
 
 import argparse
@@ -56,19 +74,23 @@
   def __init__(self, ptx_type):
 self.ptx_type = ptx_type
 self.llvm_type = {
-"f16" : "<2 x half>",
-"f32" : "float",
-"s32" : "i32",
-"s8"  : "i32",
-"u8"  : "i32",
-"s4"  : "i32",
-"u4"  : "i32",
-"b1"  : "i32",
+"f16"  : "<2 x half>",
+"f32"  : "float",
+"f64"  : "double",
+"s32"  : "i32",
+"s8"   : "i32",
+"u8"   : "i32",
+"s4"   : "i32",
+"u4"   : "i32",
+"b1"   : "i32",
+"bf16" : "i32",
+"tf32" : "i32",
 }[ptx_type];
 
 self.ptx_reg_pattern = {
 "f16" : "%hh[0-9]+",
 "f32" : "%f[0-9]+",
+"f64" : "%fd[0-9]+",

[PATCH] D104847: [Clang][NVPTX] Add NVPTX intrinsics and builtins for CUDA PTX 6.5 and 7.0 WMMA and MMA instructions

[Steffen Larsen via Phabricator via cfe-commits]

steffenlarsen marked 4 inline comments as done.
steffenlarsen added a comment.

@tra Thank you for the quick response! I agree with your comments and have made 
changes accordingly.




Comment at: clang/include/clang/Basic/BuiltinsNVPTX.def:762
 
+// Builtins to support double and alternate float WMMA instructions on sm_80
+TARGET_BUILTIN(__dmma_m8n8k4_ld_a, "vd*dC*UiIi", "", AND(SM_80,PTX70))

tra wrote:
> Is this a sufficient set of builtins to compile mma.h in CUDA-11.x?
mma.h was my frame-of-reference for the builtins and I have CUDA 11.3 
(465.19.01) installed, so I would expect it to be.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:16411-16430
 #define MMA_VARIANTS(geom, type) {{ \
+  Intrinsic::nvvm_wmma_##geom##_mma_row_row_##type, \
+  0,\
+  Intrinsic::nvvm_wmma_##geom##_mma_row_col_##type, \
+  0,\
+  Intrinsic::nvvm_wmma_##geom##_mma_col_row_##type, \
+  0,\

tra wrote:
> Nit: satf variants are in the minority. We could  move them to the end of the 
> variant list and rely on default initialization to 0. E.g something like this:
> 
> ```
> unsigned getMMAIntrinsic(int Layout, bool Satf) {
> unsigned Index = Layout + 4*Satf;
> if (Index >= Variants.size())
>   return 0;
> return Variants[Index];
>   }
> #define MMA_VARIANTS(geom, type) 
>   Intrinsic::nvvm_wmma_##geom##_mma_row_row_##type, \
>   Intrinsic::nvvm_wmma_##geom##_mma_row_col_##type, \
>   Intrinsic::nvvm_wmma_##geom##_mma_col_row_##type, \
>   Intrinsic::nvvm_wmma_##geom##_mma_col_col_##type 
> 
> #define MMA_SATF_VARIANTS(geom, type)
>   MMA_VARIANTS(geom, type), \
>   Intrinsic::nvvm_wmma_##geom##_mma_row_row_##type##_satfinite, \
>   Intrinsic::nvvm_wmma_##geom##_mma_row_col_##type##_satfinite, \
>   Intrinsic::nvvm_wmma_##geom##_mma_col_row_##type##_satfinite, \
>   Intrinsic::nvvm_wmma_##geom##_mma_col_col_##type##_satfinite 
> ...
> case NVPTX::BI__hmma_m16n16k16_mma_f16f16:
>   return {8, 8, 4, 4, {{ MMA_SATF_VARIANTS(m16n16k16, f16_f16) }}};
> ...
> case NVPTX::BI__dmma_m8n8k4_mma_f64:
>   return {1, 1, 2, 2, {{MMA_VARIANTS(m8n8k4, f64)}}};
> 
> ```
> 
> Up to you.
I agree, I like this better. In case other options will use the satf parameter 
(e.g. rnd which isn't indicated as being in use from mma.h) this solution is 
also easier to extend in the future.



Comment at: clang/test/CodeGen/builtins-nvptx-mma.py:142-146
+  elif frag.geom == "m16n16k8":
+if frag.frag == "d":
+  prefix = "__mma"
+else:
+  prefix = "__mma_tf32"

tra wrote:
> It's not obvious why  frag `d` is `__mma` and not `__mma_tf32` 
> Can we use frag.ptx_type to make that decision?
We absolutely can. I don't know why that wasn't my first solution.



Comment at: llvm/include/llvm/IR/IntrinsicsNVVM.td:4474
+foreach satf = [0, 1] in {
+  foreach rnd = ["-", "rn", "rz", "rm", "rp"] in {
+foreach op = NVVM_MMA_OPS.all_wmma_ops in {

tra wrote:
> We're often using an empty string to represent a `none`. Comparisons with `-` 
> where we check `rnd` look like we're doing something special there.
> I'd use an empty string for `rnd`, too. 
> 
Empty string works for me. I think there are/were some places that used "-" as 
a default parameter meaning `none`, but I agree with your assessment.


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

https://reviews.llvm.org/D104847

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D104847: [Clang][NVPTX] Add NVPTX intrinsics and builtins for CUDA PTX 6.5 and 7.0 WMMA and MMA instructions

[Steffen Larsen via Phabricator via cfe-commits]

steffenlarsen updated this revision to Diff 354814.
steffenlarsen added a comment.

Added comment about variant ordering.


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

https://reviews.llvm.org/D104847

Files:
  clang/include/clang/Basic/BuiltinsNVPTX.def
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/test/CodeGen/builtins-nvptx-mma.cu
  clang/test/CodeGen/builtins-nvptx-mma.py
  llvm/include/llvm/IR/IntrinsicsNVVM.td
  llvm/lib/Target/NVPTX/NVPTXISelLowering.cpp
  llvm/lib/Target/NVPTX/NVPTXInstrInfo.td
  llvm/lib/Target/NVPTX/NVPTXIntrinsics.td
  llvm/test/CodeGen/NVPTX/lit.local.cfg
  llvm/test/CodeGen/NVPTX/wmma.py

Index: llvm/test/CodeGen/NVPTX/wmma.py
===
--- llvm/test/CodeGen/NVPTX/wmma.py
+++ llvm/test/CodeGen/NVPTX/wmma.py
@@ -6,7 +6,7 @@
 # RUN: FileCheck %t-ptx60-sm_70.ll < %t-ptx60-sm_70.ll \
 # RUN:   --check-prefixes=INTRINSICS,M16N16
 # RUN: FileCheck %t-ptx60-sm_70.ll < %t-ptx60-sm_70.ll \
-# RUN:   --check-prefixes=INTRINSICS,NOEXTGEOM,NOINT,NOSUBINT,NOMMA
+# RUN:   --check-prefixes=INTRINSICS,NOEXTGEOM,NOINT,NOSUBINT,NOMMA,NODOUBLE,NOALTFLOAT
 # RUN: llc < %t-ptx60-sm_70.ll -march=nvptx64 -mcpu=sm_70 -mattr=+ptx60 \
 # RUN:   | FileCheck %t-ptx60-sm_70.ll
 
@@ -15,7 +15,7 @@
 # RUN: FileCheck %t-ptx61-sm_70.ll < %t-ptx61-sm_70.ll \
 # RUN:   --check-prefixes=INTRINSICS,M16N16,EXTGEOM
 # RUN: FileCheck %t-ptx61-sm_70.ll < %t-ptx61-sm_70.ll \
-# RUN:   --check-prefixes=INTRINSICS,NOINT,NOSUBINT,NOMMA
+# RUN:   --check-prefixes=INTRINSICS,NOINT,NOSUBINT,NOMMA,NODOUBLE,NOALTFLOAT
 # RUN: llc < %t-ptx61-sm_70.ll -march=nvptx64 -mcpu=sm_70 -mattr=+ptx61 \
 # RUN:   | FileCheck %t-ptx61-sm_70.ll
 
@@ -24,7 +24,7 @@
 # RUN: FileCheck %t-ptx63-sm_72.ll < %t-ptx63-sm_72.ll \
 # RUN:   --check-prefixes=INTRINSICS,M16N16,EXTGEOM,INT
 # RUN: FileCheck %t-ptx63-sm_72.ll < %t-ptx63-sm_72.ll \
-# RUN:   --check-prefixes=INTRINSICS,NOSUBINT,NOMMA
+# RUN:   --check-prefixes=INTRINSICS,NOSUBINT,NOMMA,NODOUBLE,NOALTFLOAT
 # RUN: llc < %t-ptx63-sm_72.ll -march=nvptx64 -mcpu=sm_72 -mattr=+ptx63 \
 # RUN:   | FileCheck %t-ptx63-sm_72.ll
 
@@ -33,7 +33,7 @@
 # RUN: FileCheck %t-ptx63-sm_75.ll < %t-ptx63-sm_75.ll \
 # RUN:   --check-prefixes=INTRINSICS,M16N16,EXTGEOM,INT,SUBINT
 # RUN: FileCheck %t-ptx63-sm_75.ll < %t-ptx63-sm_75.ll \
-# RUN:   --check-prefixes=INTRINSICS,NOMMA
+# RUN:   --check-prefixes=INTRINSICS,NOMMA,NODOUBLE,NOALTFLOAT
 # RUN: llc < %t-ptx63-sm_75.ll -march=nvptx64 -mcpu=sm_75 -mattr=+ptx63 \
 # RUN:   | FileCheck %t-ptx63-sm_75.ll
 
@@ -42,10 +42,28 @@
 # RUN: FileCheck %t-ptx64-sm_70.ll < %t-ptx64-sm_70.ll \
 # RUN:   --check-prefixes=INTRINSICS,M16N16,EXTGEOM,MMA
 # RUN: FileCheck %t-ptx64-sm_70.ll < %t-ptx64-sm_70.ll \
-# RUN:   --check-prefixes=INTRINSICS,NOINT,NOSUBINT
+# RUN:   --check-prefixes=INTRINSICS,NOINT,NOSUBINT,NODOUBLE,NOALTFLOAT
 # RUN: llc < %t-ptx64-sm_70.ll -march=nvptx64 -mcpu=sm_70 -mattr=+ptx64 \
 # RUN:   | FileCheck %t-ptx64-sm_70.ll
 
+# Check all variants of instructions supported by PTX65 on SM75+
+# RUN: python %s --ptx=65 --gpu-arch=75 > %t-ptx65-sm_75.ll
+# RUN: FileCheck %t-ptx65-sm_75.ll < %t-ptx65-sm_75.ll \
+# RUN:   --check-prefixes=INTRINSICS,M16N16,EXTGEOM,INT,SUBINT,MMA,PTX65MMA
+# RUN: FileCheck %t-ptx65-sm_75.ll < %t-ptx65-sm_75.ll \
+# RUN:   --check-prefixes=INTRINSICS
+# RUN: llc < %t-ptx65-sm_75.ll -march=nvptx64 -mcpu=sm_75 -mattr=+ptx65 \
+# RUN:   | FileCheck %t-ptx65-sm_75.ll
+
+# Check all variants of instructions supported by PTX70 on SM80+
+# RUN: python %s --ptx=70 --gpu-arch=80 > %t-ptx70-sm_80.ll
+# RUN: FileCheck %t-ptx70-sm_80.ll < %t-ptx70-sm_80.ll \
+# RUN:   --check-prefixes=INTRINSICS,M16N16,EXTGEOM,INT,SUBINT,MMA,ALTFLOAT,DOUBLE,PTX65MMA,PTX70MMA
+# RUN: FileCheck %t-ptx70-sm_80.ll < %t-ptx70-sm_80.ll \
+# RUN:   --check-prefixes=INTRINSICS
+# RUN: llc < %t-ptx70-sm_80.ll -march=nvptx64 -mcpu=sm_80 -mattr=+ptx70 \
+# RUN:   | FileCheck %t-ptx70-sm_80.ll
+
 from __future__ import print_function
 
 import argparse
@@ -56,19 +74,23 @@
   def __init__(self, ptx_type):
 self.ptx_type = ptx_type
 self.llvm_type = {
-"f16" : "<2 x half>",
-"f32" : "float",
-"s32" : "i32",
-"s8"  : "i32",
-"u8"  : "i32",
-"s4"  : "i32",
-"u4"  : "i32",
-"b1"  : "i32",
+"f16"  : "<2 x half>",
+"f32"  : "float",
+"f64"  : "double",
+"s32"  : "i32",
+"s8"   : "i32",
+"u8"   : "i32",
+"s4"   : "i32",
+"u4"   : "i32",
+"b1"   : "i32",
+"bf16" : "i32",
+"tf32" : "i32",
 }[ptx_type];
 
 self.ptx_reg_pattern = {
 "f16" : "%hh[0-9]+",
 "f32" : "%f[0-9]+",
+"f64" : "%fd[0-9]+",
 }.get(pt

[PATCH] D104847: [Clang][NVPTX] Add NVPTX intrinsics and builtins for CUDA PTX 6.5 and 7.0 WMMA and MMA instructions

[Steffen Larsen via Phabricator via cfe-commits]

steffenlarsen marked an inline comment as done.
steffenlarsen added a comment.

In D104847#2841242 , @tra wrote:

> LGTM. Would you like me to land the patch on your behalf?

If it isn't to much of a bother. Thank you. 😄


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

https://reviews.llvm.org/D104847

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D104847: [Clang][NVPTX] Add NVPTX intrinsics and builtins for CUDA PTX 6.5 and 7.0 WMMA and MMA instructions

[Steffen Larsen via Phabricator via cfe-commits]

steffenlarsen added a comment.

Sorry for the late response. Looks like you have handled the issues and more in 
your patch. Thank you for fixing my blunders. 😄




Comment at: clang/include/clang/Basic/BuiltinsNVPTX.def:727
 TARGET_BUILTIN(__bmma_m8n8k128_ld_c, "vi*iC*UiIi", "", AND(SM_75,PTX63))
 TARGET_BUILTIN(__bmma_m8n8k128_mma_xor_popc_b1, "vi*iC*iC*iC*Ii", "", 
AND(SM_75,PTX63))
 TARGET_BUILTIN(__bmma_m8n8k128_st_c_i32, "vi*iC*UiIi", "", AND(SM_75,PTX63))

tra wrote:
> Bummer. mma.h in CUDA-11.3 still does not compile for Ampere.
> 
> We appear to be missing the new `__bmma_m8n8k128_mma_and_popc_b1` builtin for 
> the `.and` variant of 1-bit `mma` introduced in PTX 7.1 and not included in 
> this patch.
> 
> https://docs.nvidia.com/cuda/parallel-thread-execution/index.html#warp-level-matrix-instructions-wmma-mma
> 
> Do you, by any chance, have upcoming patch for PTX7.1, too. :-) 
Haha, I didn't think of that one. Sadly we don't have any plans to work on 
extending support for PTX 7.1+ in the next couple of months, but it looks like 
your new patch (D105384) takes care of it anyway. 😆 



Comment at: clang/test/CodeGen/builtins-nvptx-mma.cu:781-786
+  // CHECK_PTX70_SM80: call {{.*}} 
@llvm.nvvm.wmma.m16n16k8.load.c.col.stride.f32
+  // expected-error-re@+1 {{'__mma_tf32_m16n16k8_ld_c' needs target feature 
(sm_80{{.*}},(ptx70{{.*
+  __mma_tf32_m16n16k8_ld_c(fdst, fsrc, ldm, 1);
+  // CHECK_PTX70_SM80: call {{.*}} 
@llvm.nvvm.wmma.m16n16k8.load.c.row.stride.f32
+  // expected-error-re@+1 {{'__mma_tf32_m16n16k8_ld_c' needs target feature 
(sm_80{{.*}},(ptx70{{.*
+  __mma_tf32_m16n16k8_ld_c(fdst, fsrc, ldm, 0);

tra wrote:
> tra wrote:
> > This looks rather odd. We're calling a `tf32` builtin, but expect to see 
> > and `f32` load intrinsic. Is that expected ? 
> > 
> > 
> Never mind. I think I understand what's going on now.
> CUDA headers use  __mma_tf32 builtins. `A` and `B` operate on opaque integer 
> types. `C` and `D` operate on floats.
> However, on the PTX front we have `wmma.load.{a,b}...tf32` but 
> `wmma.load.c...f32`.
> 
> I guess it does make sense to keep LLVM intrinsic names close to the 
> instructions they produce.
> 
> 
> 
Yeah, it was definitely confusing to write. I think the current state is the 
best solution, as it prioritizes consistency within the sub-projects. Not a big 
fan of the inconsistency though, but if we want to follow CUDA's example I 
suppose we're stuck with this.



Comment at: clang/test/CodeGen/builtins-nvptx-mma.py:74
+  make_ldst_ops(["m8n8k4"], ["a", "b", "c", "d"], ["f64"]) +
+  make_ldst_ops(["m16n16k8"], ["a", "b"], ["tf32"]) +
+  make_ldst_ops(["m16n16k8"], ["c", "d"], ["f32"]))

tra wrote:
> This does not seem to match the generated `builtins-nvptx-mma.cu` which does 
> have `__mma_tf32_m16n16k8_ld_c`
> If I regenrate the test I see a somewhat different set of tests, possibly 
> related to the oddity I've pointed in the generated test changes in this 
> patch.
You are absolutely right. That's a mistake on my part. Looks like you've got it 
under control in your patch. Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104847

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D105384: [NVPTX, CUDA] Add .and.popc variant of the b1 MMA instruction.

[Steffen Larsen via Phabricator via cfe-commits]

steffenlarsen requested changes to this revision.
steffenlarsen added a comment.
This revision now requires changes to proceed.

Good stuff! Thanks for adding this and adjusting the test generator. I have 
requested some minor changes, though nothing critical. Are the test failures 
related to these changes?




Comment at: clang/test/CodeGen/builtins-nvptx-mma.py:35-38
+def make_mma_ops(geoms, types_a, types_b, types_c, types_d, b1ops=None):
   ops = []
+  if b1ops is None:
+b1ops = [""]





Comment at: clang/test/CodeGen/builtins-nvptx-mma.py:84
+  # It uses __mma_tf32_m16n16k8_ld_c but __mma_m16n16k8_st_c_f32.
+  make_ldst_ops(["m16n16k8"], ["a", "b", "c", "d"], ["tf32", "f32"]))
 

The following changes would remove the need for the `m16n16k8` cases in 
`is_ldst_variant_supported`.



Comment at: clang/test/CodeGen/builtins-nvptx-mma.py:265
 min_ptx = get_required_ptx(frag)
+# TF32 uses t32 for loads.
+if frag.geom == "m16n16k8" and frag.frag =="c":




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105384

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D105384: [NVPTX, CUDA] Add .and.popc variant of the b1 MMA instruction.

[Steffen Larsen via Phabricator via cfe-commits]

steffenlarsen accepted this revision.
steffenlarsen added a comment.
This revision is now accepted and ready to land.

Thank you for addressing my concerns. I am happy with the changes. Great work!




Comment at: clang/test/CodeGen/builtins-nvptx-mma.py:35-38
+def make_mma_ops(geoms, types_a, types_b, types_c, types_d, b1ops=None):
   ops = []
+  if b1ops is None:
+b1ops = [""]

tra wrote:
> steffenlarsen wrote:
> > 
> Default initializers that use objects in python are one of the common gotchas.
> https://docs.python-guide.org/writing/gotchas/#mutable-default-arguments
> 
> It probably does not make much of a difference in this case as we do not 
> modify it, but I'd prefer to avoid it nevertheless.
Interesting and a little horrifying. I agree it wouldn't have been a problem in 
this particular case, but I understand your concern and I am fine with keeping 
it as-is. 👍 



Comment at: clang/test/CodeGen/builtins-nvptx-mma.py:84
+  # It uses __mma_tf32_m16n16k8_ld_c but __mma_m16n16k8_st_c_f32.
+  make_ldst_ops(["m16n16k8"], ["a", "b", "c", "d"], ["tf32", "f32"]))
 

tra wrote:
> steffenlarsen wrote:
> > The following changes would remove the need for the `m16n16k8` cases in 
> > `is_ldst_variant_supported`.
> This was done deliberately. I did have that in an earlier variant of the 
> patch, but eventually settled on the current version.
> 
> My thinking is that `make_ldst_ops(m16n16k8)` would create whatever is 
> necessary for `m16n16k8`, and we'd keep the decision of what to produce in 
> one place in `is_ldst_variant_supported`. Splitting one make_ldst_ops into 
> two here makes this decision implicit which would need additional 
> explanations. Code that needs less explanations  wins. :-)
> 
My concern is that it is more confusing this way because they are the only 
load/store ops generated where the type is then later filtered. It makes sense 
for MMA because it needs to filter out select combinations, but here the 
comment above says one thing and `make_ldst_ops` does something else.

I don't think it makes a huge difference either way, so if you prefer the 
current version I am okay with keeping it. That said, it might be good to have 
a comment in the `m16n16k8` case of `is_ldst_variant_supported` making the 
connection to this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105384

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D107054: [Clang][CUDA] Add descriptors, mappings, and features for missing CUDA and PTX versions

[Steffen Larsen via Phabricator via cfe-commits]

steffenlarsen closed this revision.
steffenlarsen marked an inline comment as done.
steffenlarsen added a comment.

In D107054#3141285 , @tra wrote:

> I think this patch has been obsoleted by https://reviews.llvm.org/D113249 
> which has already landed.
>
> My apologies for letting the patch slip through the cracks.

No harm, no foul! Thank you for pointing to the new patch. I agree, it seems 
that these changes are obsolete so I'll close this. :)


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

https://reviews.llvm.org/D107054

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114439: [Annotation] Allow parameter pack expansions in annotate attribute

[Steffen Larsen via Phabricator via cfe-commits]

steffenlarsen created this revision.
steffenlarsen added reviewers: erichkeane, aaron.ballman, Tyker, Naghasan.
Herald added a subscriber: jdoerfert.
steffenlarsen requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

These changes make the Clang parser recognize parameter pack expansion in 
attribute arguments and consume them if the attribute is marked as supporting 
parameter pack expansions.
Currently only the `clang::annotate` attribute will support parameter pack 
expansions in its arguments. The parser will issue an error diagnostic if other 
attributes are passed parameter pack expansion arguments.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D114439

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/Parser/cxx0x-attributes.cpp
  clang/test/SemaTemplate/attributes.cpp
  clang/utils/TableGen/ClangAttrEmitter.cpp

Index: clang/utils/TableGen/ClangAttrEmitter.cpp
===
--- clang/utils/TableGen/ClangAttrEmitter.cpp
+++ clang/utils/TableGen/ClangAttrEmitter.cpp
@@ -1697,6 +1697,22 @@
   OS << "#endif // CLANG_ATTR_LATE_PARSED_LIST\n\n";
 }
 
+// Emits the ParmExpansionArgsSupport property for attributes.
+static void emitClangAttrParmExpansionArgsSupportList(RecordKeeper &Records,
+  raw_ostream &OS) {
+  OS << "#if defined(CLANG_ATTR_PARM_EXPANSION_ARGS_SUPPORT_LIST)\n";
+  std::vector Attrs = Records.getAllDerivedDefinitions("Attr");
+
+  for (const auto *Attr : Attrs) {
+if (Attr->getValueAsBit("ParmExpansionArgsSupport")) {
+  std::vector Spellings = GetFlattenedSpellings(*Attr);
+  for (const auto &I : Spellings)
+OS << ".Case(\"" << I.name() << "\", true)\n";
+}
+  }
+  OS << "#endif // CLANG_ATTR_PARM_EXPANSION_ARGS_SUPPORT_LIST\n\n";
+}
+
 static bool hasGNUorCXX11Spelling(const Record &Attribute) {
   std::vector Spellings = GetFlattenedSpellings(Attribute);
   for (const auto &I : Spellings) {
@@ -4221,6 +4237,7 @@
   emitClangAttrThisIsaIdentifierArgList(Records, OS);
   emitClangAttrTypeArgList(Records, OS);
   emitClangAttrLateParsedList(Records, OS);
+  emitClangAttrParmExpansionArgsSupportList(Records, OS);
 }
 
 void EmitClangAttrSubjectMatchRulesParserStringSwitches(RecordKeeper &Records,
Index: clang/test/SemaTemplate/attributes.cpp
===
--- clang/test/SemaTemplate/attributes.cpp
+++ clang/test/SemaTemplate/attributes.cpp
@@ -64,6 +64,23 @@
 template [[clang::annotate("ANNOTATE_FOO"), clang::annotate("ANNOTATE_BAR")]] void HasAnnotations();
 void UseAnnotations() { HasAnnotations(); }
 
+// CHECK:  FunctionTemplateDecl {{.*}} HasPackAnnotations
+// CHECK:AnnotateAttr {{.*}} "ANNOTATE_BAZ"
+// CHECK:  FunctionDecl {{.*}} HasPackAnnotations
+// CHECK:TemplateArgument{{.*}} pack
+// CHECK-NEXT: TemplateArgument{{.*}} integral 1
+// CHECK-NEXT: TemplateArgument{{.*}} integral 2
+// CHECK-NEXT: TemplateArgument{{.*}} integral 3
+// CHECK:AnnotateAttr {{.*}} "ANNOTATE_BAZ"
+// CHECK:  ConstantExpr {{.*}} 'int'
+// CHECK-NEXT:   value: Int 1
+// CHECK:  ConstantExpr {{.*}} 'int'
+// CHECK-NEXT:   value: Int 2
+// CHECK:  ConstantExpr {{.*}} 'int'
+// CHECK-NEXT:   value: Int 3
+template  [[clang::annotate("ANNOTATE_BAZ", Is...)]] void HasPackAnnotations();
+void UsePackAnnotations() { HasPackAnnotations<1, 2, 3>(); }
+
 namespace preferred_name {
   int x [[clang::preferred_name("frank")]]; // expected-error {{expected a type}}
   int y [[clang::preferred_name(int)]]; // expected-warning {{'preferred_name' attribute only applies to class templates}}
Index: clang/test/Parser/cxx0x-attributes.cpp
===
--- clang/test/Parser/cxx0x-attributes.cpp
+++ clang/test/Parser/cxx0x-attributes.cpp
@@ -258,8 +258,10 @@
   [[]] return;
 }
 
-template void variadic() {
+template  void variadic() {
   void bar [[noreturn...]] (); // expected-error {{attribute 'noreturn' cannot be used as an attribute pack}}
+  void baz [[clang::no_sanitize(Is...)]] (); // expected-error {{attribute 'no_sanitize' does not support parameter pack expansion in arguments}}
+  void boo [[unknown::foo(Is...)]] ();   // expected-warning {{unknown attribute 'foo' ignored}}
 }
 
 // Expression tests
Index: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -189,13 +189,9 @@
   EnterExpressionEvaluationContext Unevaluated(
   S, Sema::ExpressionEvaluationContext::ConstantEvaluated);
   SmallVector Args;
-  Args.reserv

[PATCH] D114439: [Annotation] Allow parameter pack expansions in annotate attribute

[Steffen Larsen via Phabricator via cfe-commits]

steffenlarsen added inline comments.



Comment at: clang/include/clang/Basic/Attr.td:544-546
+  // Set to true for attributes that support parameter pack expansion in its
+  // arguments.
+  bit ParmExpansionArgsSupport = 0;

aaron.ballman wrote:
> From a design perspective, I think this is a property of the parameters of an 
> attribute rather than of an attribute itself. So I'd rather not add a bit to 
> the `Attr` class, but instead modify the `Argument` class. There are a few 
> approaches to that which I can think of, and I'm not certain which is the 
> best approach.
> 
> 1) Introduce `ExpressionPackArgument` and attributes can opt into using it if 
> they want to support packs.
> 2) Add this bit to `VariadicExprArgument` and attributes can opt into using 
> it if they want to support packs.
> 3) Change `VariadicExprArgument` to also mean "allows packs" and all 
> attributes using this automatically support packs.
> 
> I think that my conservative preference is for #2, but my intuition is that 
> #3 is where we probably ultimately want to get to.
> 
> There are definitely some open questions with this approach that we'll have 
> to figure out:
> 
> * Can you mix packs with variadics in the same attribute?
> * Can you have two pack arguments in the same attribute?
> * Does the tablegen design also seem likely to work when we want to introduce 
> type packs instead of value packs?
> 
> I think the safest bet is to be conservative and not allow mixing packs with 
> variadics, and not allowing multiple packs. We should be able to diagnose 
> that situation from ClangAttrEmitter.cpp to help attribute authors out. 
> However, it'd be worth adding a FIXME comment to that diagnostic logic asking 
> whether we want to relax the behavior at some point. But if you feel strongly 
> that we should support these situations initially, I wouldn't be opposed.
Having `ParmExpansionArgsSupport` in `Attr` would allow attributes to populate 
with parameter packs across argument boundaries. It seems more permissive, but 
frankly I have no attachment to it, so I am okay with tying it to `Argument` 
instead.

Of the mentioned options, I agree #2 and #3 are the preferred paths. For 
simplicity I prefer #2, though I will have to figure out how best to add a 
check on individual arguments, which @erichkeane's suggestion to limit the 
attributes to only allowing parameter packs in variadic arguments as the last 
argument would help with. On the other hand, the population logic for most of 
the relevant attributes are explicitly defined in 
`clang/lib/Sema/SemaDeclAttr.cpp` (which will have to be changed with #3) so 
being more permissive - like the current version of the patch is - gives more 
freedom to the attributes.

> Does the tablegen design also seem likely to work when we want to introduce 
> type packs instead of value packs?

I am not sure about how tablegen would take it, but the parser doesn't 
currently parse more than a single type argument, so I don't see a need for 
allowing type packs at the moment.

> I think the safest bet is to be conservative and not allow mixing packs with 
> variadics, and not allowing multiple packs. We should be able to diagnose 
> that situation from ClangAttrEmitter.cpp to help attribute authors out. 
> However, it'd be worth adding a FIXME comment to that diagnostic logic asking 
> whether we want to relax the behavior at some point. But if you feel strongly 
> that we should support these situations initially, I wouldn't be opposed.

If we limit packs to `VariadicExprArgument` - like `annotate` does in the 
current state of this patch - having multiple packs in the same argument is 
simple to handle. Both are represented as `Expr` and the variadic argument will 
simply contain two expressions until the templates are instantiated and the 
packs are expanded. This is also a feature I would like to keep, though it 
should be possible to work around the limitation if we conclude it can't stay.




Comment at: clang/lib/Parse/ParseDecl.cpp:447
+  }
+  ArgExpr = Actions.ActOnPackExpansion(ArgExpr.get(), ConsumeToken());
+}

erichkeane wrote:
> Do you have a test for something that isn't a pack followed by an ellipsis?  
> What is the behavior there?   I'm also concerned that there doesn't seem to 
> be anything here that handles complex-pack expansions (like folds), can you 
> test that as well?
> 
> Also, why is this 'if' not up on line 429 (that is, outside of this 'else'?). 
>  I believe ParseAssignmentExpression is going to muck-up the tokens, so I'm 
> not particularly sure what is going to happen here?
> Do you have a test for something that isn't a pack followed by an ellipsis? 
> What is the behavior there? I'm also concerned that there doesn't seem to be 
> anything here that handles complex-pack expansions (like folds), can you test 
> that as well?

Good question! I would expect it to fail 

[PATCH] D114439: [Annotation] Allow parameter pack expansions in annotate attribute

[Steffen Larsen via Phabricator via cfe-commits]

steffenlarsen added inline comments.



Comment at: clang/lib/Parse/ParseDecl.cpp:447
+  }
+  ArgExpr = Actions.ActOnPackExpansion(ArgExpr.get(), ConsumeToken());
+}

steffenlarsen wrote:
> erichkeane wrote:
> > Do you have a test for something that isn't a pack followed by an ellipsis? 
> >  What is the behavior there?   I'm also concerned that there doesn't seem 
> > to be anything here that handles complex-pack expansions (like folds), can 
> > you test that as well?
> > 
> > Also, why is this 'if' not up on line 429 (that is, outside of this 
> > 'else'?).  I believe ParseAssignmentExpression is going to muck-up the 
> > tokens, so I'm not particularly sure what is going to happen here?
> > Do you have a test for something that isn't a pack followed by an ellipsis? 
> > What is the behavior there? I'm also concerned that there doesn't seem to 
> > be anything here that handles complex-pack expansions (like folds), can you 
> > test that as well?
> 
> Good question! I would expect it to fail in kind during template 
> instantiation, or earlier if it is not an expression, but I will test it out 
> once we converge on a new solution.
> 
> > Also, why is this 'if' not up on line 429 (that is, outside of this 
> > 'else'?). I believe ParseAssignmentExpression is going to muck-up the 
> > tokens, so I'm not particularly sure what is going to happen here?
> 
> I originally tried having the `ActOnPackExpansion` earlier, but it did indeed 
> cause trouble. However, if my understanding of the previous two branches is 
> correct, they are:
> 
> 
>   # Type argument. This could be consuming the ellipsis as well, though I 
> suspect it needs another path given that it does not seem to produce any 
> expressions. Note however that it currently seems to stop argument parsing 
> after reading the first type (confirmed by the FIXME) so parameter pack logic 
> for this branch would probably be more suitable once it actually allows 
> attributes to take multiple types.
>   # Identifier argument. Correct me if I am wrong, but I don't think this can 
> be populated by a parameter pack.
> 
> I could add the diagnostic to these if ellipsis is found, but if my 
> assumptions are correct it doesn't make much sense to expand these branches 
> beyond that.
> I'm also concerned that there doesn't seem to be anything here that handles 
> complex-pack expansions (like folds), can you test that as well?

Sorry, I forgot to address this. I have not tested it, but I would expect 
complex-pack expansions like folds to be at expression-level. I will make sure 
to test this as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114439

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114439: [Annotation] Allow parameter pack expansions in annotate attribute

[Steffen Larsen via Phabricator via cfe-commits]

steffenlarsen updated this revision to Diff 391588.
steffenlarsen edited the summary of this revision.

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

https://reviews.llvm.org/D114439

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/Parser/cxx0x-attributes.cpp
  clang/test/SemaTemplate/attributes.cpp
  clang/utils/TableGen/ClangAttrEmitter.cpp

Index: clang/utils/TableGen/ClangAttrEmitter.cpp
===
--- clang/utils/TableGen/ClangAttrEmitter.cpp
+++ clang/utils/TableGen/ClangAttrEmitter.cpp
@@ -1164,10 +1164,12 @@
   };
 
   class VariadicExprArgument : public VariadicArgument {
+bool AllowPack = false;
+
   public:
 VariadicExprArgument(const Record &Arg, StringRef Attr)
-  : VariadicArgument(Arg, Attr, "Expr *")
-{}
+: VariadicArgument(Arg, Attr, "Expr *"),
+  AllowPack(Arg.getValueAsBit("AllowPack")) {}
 
 void writeASTVisitorTraversal(raw_ostream &OS) const override {
   OS << "  {\n";
@@ -2264,6 +2266,43 @@
   OS << "#endif // CLANG_ATTR_THIS_ISA_IDENTIFIER_ARG_LIST\n\n";
 }
 
+static void emitClangAttrNumArgs(RecordKeeper &Records, raw_ostream &OS) {
+  OS << "#if defined(CLANG_ATTR_NUM_ARGS)\n";
+  std::vector Attrs = Records.getAllDerivedDefinitions("Attr");
+  for (const auto *A : Attrs) {
+std::vector Args = A->getValueAsListOfDefs("Args");
+forEachUniqueSpelling(*A, [&](const FlattenedSpelling &S) {
+  OS << ".Case(\"" << S.name() << "\", " << Args.size() << ")\n";
+});
+  }
+  OS << "#endif // CLANG_ATTR_NUM_ARGS\n\n";
+}
+
+static bool argSupportsParmPack(const Record *Arg) {
+  return !Arg->getSuperClasses().empty() &&
+ llvm::StringSwitch(
+ Arg->getSuperClasses().back().first->getName())
+ .Case("VariadicExprArgument", true)
+ .Default(false) &&
+ Arg->getValueAsBit("AllowPack");
+}
+
+static void emitClangAttrLastArgParmPackSupport(RecordKeeper &Records,
+raw_ostream &OS) {
+  OS << "#if defined(CLANG_ATTR_LAST_ARG_PARM_PACK_SUPPORT)\n";
+  std::vector Attrs = Records.getAllDerivedDefinitions("Attr");
+  for (const auto *A : Attrs) {
+std::vector Args = A->getValueAsListOfDefs("Args");
+bool LastArgSupportsParmPack =
+!Args.empty() && argSupportsParmPack(Args.back());
+forEachUniqueSpelling(*A, [&](const FlattenedSpelling &S) {
+  OS << ".Case(\"" << S.name() << "\", " << LastArgSupportsParmPack
+ << ")\n";
+});
+  }
+  OS << "#endif // CLANG_ATTR_LAST_ARG_PARM_PACK_SUPPORT\n\n";
+}
+
 static void emitAttributes(RecordKeeper &Records, raw_ostream &OS,
bool Header) {
   std::vector Attrs = Records.getAllDerivedDefinitions("Attr");
@@ -4219,6 +4258,8 @@
   emitClangAttrIdentifierArgList(Records, OS);
   emitClangAttrVariadicIdentifierArgList(Records, OS);
   emitClangAttrThisIsaIdentifierArgList(Records, OS);
+  emitClangAttrNumArgs(Records, OS);
+  emitClangAttrLastArgParmPackSupport(Records, OS);
   emitClangAttrTypeArgList(Records, OS);
   emitClangAttrLateParsedList(Records, OS);
 }
Index: clang/test/SemaTemplate/attributes.cpp
===
--- clang/test/SemaTemplate/attributes.cpp
+++ clang/test/SemaTemplate/attributes.cpp
@@ -64,6 +64,23 @@
 template [[clang::annotate("ANNOTATE_FOO"), clang::annotate("ANNOTATE_BAR")]] void HasAnnotations();
 void UseAnnotations() { HasAnnotations(); }
 
+// CHECK:  FunctionTemplateDecl {{.*}} HasPackAnnotations
+// CHECK:AnnotateAttr {{.*}} "ANNOTATE_BAZ"
+// CHECK:  FunctionDecl {{.*}} HasPackAnnotations
+// CHECK:TemplateArgument{{.*}} pack
+// CHECK-NEXT: TemplateArgument{{.*}} integral 1
+// CHECK-NEXT: TemplateArgument{{.*}} integral 2
+// CHECK-NEXT: TemplateArgument{{.*}} integral 3
+// CHECK:AnnotateAttr {{.*}} "ANNOTATE_BAZ"
+// CHECK:  ConstantExpr {{.*}} 'int'
+// CHECK-NEXT:   value: Int 1
+// CHECK:  ConstantExpr {{.*}} 'int'
+// CHECK-NEXT:   value: Int 2
+// CHECK:  ConstantExpr {{.*}} 'int'
+// CHECK-NEXT:   value: Int 3
+template  [[clang::annotate("ANNOTATE_BAZ", Is...)]] void HasPackAnnotations();
+void UsePackAnnotations() { HasPackAnnotations<1, 2, 3>(); }
+
 namespace preferred_name {
   int x [[clang::preferred_name("frank")]]; // expected-error {{expected a type}}
   int y [[clang::preferred_name(int)]]; // expected-warning {{'preferred_name' attribute only applies to class templates}}
Index: clang/test/Parser/cxx0x-attributes.cpp
===
--- clang/test/Parser/cxx0x-attributes.cpp
+++ clang/test/Parser/cxx0x-attributes.cpp
@@ -258,8 +258,14 @@
   [[]] return;
 }
 
-template void variadic() {
-  void bar 

[PATCH] D114439: [Annotation] Allow parameter pack expansions in annotate attribute

[Steffen Larsen via Phabricator via cfe-commits]

steffenlarsen added a comment.

Thank you both for the reviews! Consensus seems to be having support for pack 
expansion at argument level for now and let more complicated logic be added 
when there is an actual need. I have applied these changes as I understood them 
and have added/adjusted the requested test cases. Please have a look and let me 
know what you think. 😄

> That's what I want to avoid. :-D I would prefer that the arguments have to 
> opt into that behavior because the alternative could get ambiguous. I'd 
> rather we do some extra work to explicitly support that situation once we 
> have a specific attribute in mind for it.

I'm okay with that! I have made the changes to only allow it in the last 
argument if it is marked as supporting pack expansion. Note that it assumes 
that there are no other variadic parameters except the last one, as suggested.

> Hmm, let's make sure we're on the same page. The situations I think we should 
> avoid are:
>
>   // Mixing variadics and packs.
>   let Args = [VariadicExprArgument<"VarArgs">, VariadicExprArgument<"Pack", 
> /*AllowPack*/1>];
>   
>   // Multiple packs.
>   let Args = [VariadicExprArgument<"FirstPack", /*AllowPack*/1>, 
> VariadicExprArgument<"SecondPack", /*AllowPack*/1>];

Oh, I see what you mean. Yeah I agree for sure. I think the only exceptions we 
currently have to this is `omp` attributes, but given that they are pragmas 
they should be nowhere close to this.


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

https://reviews.llvm.org/D114439

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114439: [Annotation] Allow parameter pack expansions in annotate attribute

[Steffen Larsen via Phabricator via cfe-commits]

steffenlarsen added a comment.

Thank you, @aaron.ballman ! I will update the revision in a moment with some of 
the suggested changes.




Comment at: clang/lib/Parse/ParseDecl.cpp:436-438
+// Interpret "kw_this" as an identifier if the attributed requests it.
+if (ChangeKWThisToIdent && Tok.is(tok::kw_this))
+  Tok.setKind(tok::identifier);

aaron.ballman wrote:
> I'm a bit surprised this logic moved -- are there no times when we'd need it 
> within the new `else` clause?
This is done because `ParseExpressionList` wouldn't know what to do with 
identifiers, so it keeps the old logic. This means that attributes that consist 
of a variadic identifier argument cannot parse packs and initializer lists. It 
shouldn't be a problem for now, but it does kill some generality.

An alternative is to split the individual expression parsing from 
`ParseExpressionList` and use that in here in a similar way to how it parsed 
before. Would that be preferred?



Comment at: clang/lib/Parse/ParseExpr.cpp:3360
+ llvm::function_ref ExpressionStarts,
+ bool FailImmediatelyOnInvalidExpr,
+ bool EarlyTypoCorrection) {

aaron.ballman wrote:
> or something along those lines (note, the logic has reversed meaning). "Fail 
> immediately" doesn't quite convey the behavior to me because we fail 
> immediately either way, it's more about whether we attempt to skip tokens to 
> get to the end of the expression list. I'm not strongly tied to the name I 
> suggested either, btw.
> "Fail immediately" doesn't quite convey the behavior to me because we fail 
> immediately either way

Am I misunderstanding `FailUntil` or is the intention of `SkipUntil(tok::comma, 
tok::r_paren, StopBeforeMatch)` to either stop at the next "," or ")"? If I am 
not misunderstanding, I believe it will continue parsing expressions after 
comma if `SkipUntil` stopped before it. As such I don't think 
`FailImmediatelyOnInvalidExpr` is inherently wrong, but I agree that skipping 
the skipping isn't very well conveyed by the name as is.



Comment at: clang/lib/Parse/ParseExpr.cpp:3374-3375
 
+if (EarlyTypoCorrection)
+  Expr = Actions.CorrectDelayedTyposInExpr(Expr);
+

aaron.ballman wrote:
> Not that I think this is a bad change, but it seems unrelated to the patch. 
> Can you explain why you made the change?
Admittedly I am not sure why it is needed, but in the previous version of the 
parsing for attribute parameters it does the typo-correction immediately after 
parsing the expression and if this isn't added a handful of tests fail.


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

https://reviews.llvm.org/D114439

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114439: [Annotation] Allow parameter pack expansions in annotate attribute

[Steffen Larsen via Phabricator via cfe-commits]

steffenlarsen updated this revision to Diff 401580.
steffenlarsen added a comment.

Applied suggestions.


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

https://reviews.llvm.org/D114439

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/include/clang/Parse/Parser.h
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Parse/ParseExpr.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/Parser/cxx0x-attributes.cpp
  clang/test/SemaTemplate/attributes.cpp
  clang/utils/TableGen/ClangAttrEmitter.cpp

Index: clang/utils/TableGen/ClangAttrEmitter.cpp
===
--- clang/utils/TableGen/ClangAttrEmitter.cpp
+++ clang/utils/TableGen/ClangAttrEmitter.cpp
@@ -1164,10 +1164,12 @@
   };
 
   class VariadicExprArgument : public VariadicArgument {
+bool AllowPack = false;
+
   public:
 VariadicExprArgument(const Record &Arg, StringRef Attr)
-  : VariadicArgument(Arg, Attr, "Expr *")
-{}
+: VariadicArgument(Arg, Attr, "Expr *"),
+  AllowPack(Arg.getValueAsBit("AllowPack")) {}
 
 void writeASTVisitorTraversal(raw_ostream &OS) const override {
   OS << "  {\n";
@@ -2264,6 +2266,47 @@
   OS << "#endif // CLANG_ATTR_THIS_ISA_IDENTIFIER_ARG_LIST\n\n";
 }
 
+static void emitClangAttrNumArgs(RecordKeeper &Records, raw_ostream &OS) {
+  OS << "#if defined(CLANG_ATTR_NUM_ARGS)\n";
+  std::vector Attrs = Records.getAllDerivedDefinitions("Attr");
+  for (const auto *A : Attrs) {
+std::vector Args = A->getValueAsListOfDefs("Args");
+forEachUniqueSpelling(*A, [&](const FlattenedSpelling &S) {
+  OS << ".Case(\"" << S.name() << "\", " << Args.size() << ")\n";
+});
+  }
+  OS << "#endif // CLANG_ATTR_NUM_ARGS\n\n";
+}
+
+static bool argSupportsParmPack(const Record *Arg) {
+  return !Arg->getSuperClasses().empty() &&
+ llvm::StringSwitch(
+ Arg->getSuperClasses().back().first->getName())
+ .Case("VariadicExprArgument", true)
+ .Default(false) &&
+ Arg->getValueAsBit("AllowPack");
+}
+
+static void emitClangAttrLastArgParmPackSupport(RecordKeeper &Records,
+raw_ostream &OS) {
+  OS << "#if defined(CLANG_ATTR_LAST_ARG_PARM_PACK_SUPPORT)\n";
+  std::vector Attrs = Records.getAllDerivedDefinitions("Attr");
+  for (const auto *A : Attrs) {
+std::vector Args = A->getValueAsListOfDefs("Args");
+bool LastArgSupportsParmPack =
+!Args.empty() && argSupportsParmPack(Args.back());
+forEachUniqueSpelling(*A, [&](const FlattenedSpelling &S) {
+  OS << ".Case(\"" << S.name() << "\", " << LastArgSupportsParmPack
+ << ")\n";
+});
+  }
+  OS << "#endif // CLANG_ATTR_LAST_ARG_PARM_PACK_SUPPORT\n\n";
+}
+
+static bool isArgVariadic(const Record &R, StringRef AttrName) {
+  return createArgument(R, AttrName)->isVariadic();
+}
+
 static void emitAttributes(RecordKeeper &Records, raw_ostream &OS,
bool Header) {
   std::vector Attrs = Records.getAllDerivedDefinitions("Attr");
@@ -2320,6 +2363,18 @@
 std::vector> Args;
 Args.reserve(ArgRecords.size());
 
+if (!ArgRecords.empty()) {
+  const bool LastArgSupportsPack = argSupportsParmPack(ArgRecords.back());
+  for (size_t I = 0; I < ArgRecords.size() - 1; ++I) {
+assert((!LastArgSupportsPack ||
+!isArgVariadic(*ArgRecords[I], R.getName())) &&
+   "Attributes supporting packs can only have the last "
+   "argument as variadic");
+assert(!argSupportsParmPack(ArgRecords[I]) &&
+   "Only the last argument can allow packs");
+  }
+}
+
 bool HasOptArg = false;
 bool HasFakeArg = false;
 for (const auto *ArgRecord : ArgRecords) {
@@ -3382,10 +3437,6 @@
   }
 }
 
-static bool isArgVariadic(const Record &R, StringRef AttrName) {
-  return createArgument(R, AttrName)->isVariadic();
-}
-
 static void emitArgInfo(const Record &R, raw_ostream &OS) {
   // This function will count the number of arguments specified for the
   // attribute and emit the number of required arguments followed by the
@@ -4219,6 +4270,8 @@
   emitClangAttrIdentifierArgList(Records, OS);
   emitClangAttrVariadicIdentifierArgList(Records, OS);
   emitClangAttrThisIsaIdentifierArgList(Records, OS);
+  emitClangAttrNumArgs(Records, OS);
+  emitClangAttrLastArgParmPackSupport(Records, OS);
   emitClangAttrTypeArgList(Records, OS);
   emitClangAttrLateParsedList(Records, OS);
 }
Index: clang/test/SemaTemplate/attributes.cpp
===
--- clang/test/SemaTemplate/attributes.cpp
+++ clang/test/SemaTemplate/attributes.cpp
@@ -64,6 +64,23 @@
 template [[clang::annotate("ANNOTATE_FOO"), clang::annotate("ANNOTATE_BAR")]] void HasAnnotations();
 void UseAnnotations() { HasAnnotations(); }
 
+// CHECK:  FunctionTemplateDecl {{.*}} HasPackAnnotation

[PATCH] D114439: [Annotation] Allow parameter pack expansions in annotate attribute

[Steffen Larsen via Phabricator via cfe-commits]

steffenlarsen updated this revision to Diff 401910.
steffenlarsen added a comment.

Moved comment.


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

https://reviews.llvm.org/D114439

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/include/clang/Parse/Parser.h
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Parse/ParseExpr.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/Parser/cxx0x-attributes.cpp
  clang/test/SemaTemplate/attributes.cpp
  clang/utils/TableGen/ClangAttrEmitter.cpp

Index: clang/utils/TableGen/ClangAttrEmitter.cpp
===
--- clang/utils/TableGen/ClangAttrEmitter.cpp
+++ clang/utils/TableGen/ClangAttrEmitter.cpp
@@ -1164,10 +1164,12 @@
   };
 
   class VariadicExprArgument : public VariadicArgument {
+bool AllowPack = false;
+
   public:
 VariadicExprArgument(const Record &Arg, StringRef Attr)
-  : VariadicArgument(Arg, Attr, "Expr *")
-{}
+: VariadicArgument(Arg, Attr, "Expr *"),
+  AllowPack(Arg.getValueAsBit("AllowPack")) {}
 
 void writeASTVisitorTraversal(raw_ostream &OS) const override {
   OS << "  {\n";
@@ -2264,6 +2266,47 @@
   OS << "#endif // CLANG_ATTR_THIS_ISA_IDENTIFIER_ARG_LIST\n\n";
 }
 
+static void emitClangAttrNumArgs(RecordKeeper &Records, raw_ostream &OS) {
+  OS << "#if defined(CLANG_ATTR_NUM_ARGS)\n";
+  std::vector Attrs = Records.getAllDerivedDefinitions("Attr");
+  for (const auto *A : Attrs) {
+std::vector Args = A->getValueAsListOfDefs("Args");
+forEachUniqueSpelling(*A, [&](const FlattenedSpelling &S) {
+  OS << ".Case(\"" << S.name() << "\", " << Args.size() << ")\n";
+});
+  }
+  OS << "#endif // CLANG_ATTR_NUM_ARGS\n\n";
+}
+
+static bool argSupportsParmPack(const Record *Arg) {
+  return !Arg->getSuperClasses().empty() &&
+ llvm::StringSwitch(
+ Arg->getSuperClasses().back().first->getName())
+ .Case("VariadicExprArgument", true)
+ .Default(false) &&
+ Arg->getValueAsBit("AllowPack");
+}
+
+static void emitClangAttrLastArgParmPackSupport(RecordKeeper &Records,
+raw_ostream &OS) {
+  OS << "#if defined(CLANG_ATTR_LAST_ARG_PARM_PACK_SUPPORT)\n";
+  std::vector Attrs = Records.getAllDerivedDefinitions("Attr");
+  for (const auto *A : Attrs) {
+std::vector Args = A->getValueAsListOfDefs("Args");
+bool LastArgSupportsParmPack =
+!Args.empty() && argSupportsParmPack(Args.back());
+forEachUniqueSpelling(*A, [&](const FlattenedSpelling &S) {
+  OS << ".Case(\"" << S.name() << "\", " << LastArgSupportsParmPack
+ << ")\n";
+});
+  }
+  OS << "#endif // CLANG_ATTR_LAST_ARG_PARM_PACK_SUPPORT\n\n";
+}
+
+static bool isArgVariadic(const Record &R, StringRef AttrName) {
+  return createArgument(R, AttrName)->isVariadic();
+}
+
 static void emitAttributes(RecordKeeper &Records, raw_ostream &OS,
bool Header) {
   std::vector Attrs = Records.getAllDerivedDefinitions("Attr");
@@ -2320,6 +2363,18 @@
 std::vector> Args;
 Args.reserve(ArgRecords.size());
 
+if (!ArgRecords.empty()) {
+  const bool LastArgSupportsPack = argSupportsParmPack(ArgRecords.back());
+  for (size_t I = 0; I < ArgRecords.size() - 1; ++I) {
+assert((!LastArgSupportsPack ||
+!isArgVariadic(*ArgRecords[I], R.getName())) &&
+   "Attributes supporting packs can only have the last "
+   "argument as variadic");
+assert(!argSupportsParmPack(ArgRecords[I]) &&
+   "Only the last argument can allow packs");
+  }
+}
+
 bool HasOptArg = false;
 bool HasFakeArg = false;
 for (const auto *ArgRecord : ArgRecords) {
@@ -3382,10 +3437,6 @@
   }
 }
 
-static bool isArgVariadic(const Record &R, StringRef AttrName) {
-  return createArgument(R, AttrName)->isVariadic();
-}
-
 static void emitArgInfo(const Record &R, raw_ostream &OS) {
   // This function will count the number of arguments specified for the
   // attribute and emit the number of required arguments followed by the
@@ -4219,6 +4270,8 @@
   emitClangAttrIdentifierArgList(Records, OS);
   emitClangAttrVariadicIdentifierArgList(Records, OS);
   emitClangAttrThisIsaIdentifierArgList(Records, OS);
+  emitClangAttrNumArgs(Records, OS);
+  emitClangAttrLastArgParmPackSupport(Records, OS);
   emitClangAttrTypeArgList(Records, OS);
   emitClangAttrLateParsedList(Records, OS);
 }
Index: clang/test/SemaTemplate/attributes.cpp
===
--- clang/test/SemaTemplate/attributes.cpp
+++ clang/test/SemaTemplate/attributes.cpp
@@ -64,6 +64,23 @@
 template [[clang::annotate("ANNOTATE_FOO"), clang::annotate("ANNOTATE_BAR")]] void HasAnnotations();
 void UseAnnotations() { HasAnnotations(); }
 
+// CHECK:  FunctionTemplateDecl {{.*}} HasPackAnnotations
+// 

[PATCH] D114439: [Annotation] Allow parameter pack expansions in annotate attribute

[Steffen Larsen via Phabricator via cfe-commits]

steffenlarsen marked an inline comment as done.
steffenlarsen added inline comments.



Comment at: clang/lib/Parse/ParseDecl.cpp:468
+
+  // General case. Parse all available expressions.
+  CommaLocsTy CommaLocs;

aaron.ballman wrote:
> I think it'd be good to move this comment up closer to the `else` on line 461 
> so that it's clear what the else clause covers (and more clear that this is 
> the common case).
I completely agree. It has been moved to right after the `else`.



Comment at: clang/lib/Parse/ParseExpr.cpp:3374-3375
 
+if (EarlyTypoCorrection)
+  Expr = Actions.CorrectDelayedTyposInExpr(Expr);
+

aaron.ballman wrote:
> steffenlarsen wrote:
> > aaron.ballman wrote:
> > > Not that I think this is a bad change, but it seems unrelated to the 
> > > patch. Can you explain why you made the change?
> > Admittedly I am not sure why it is needed, but in the previous version of 
> > the parsing for attribute parameters it does the typo-correction 
> > immediately after parsing the expression and if this isn't added a handful 
> > of tests fail.
> Ah, thanks for the info! So this is basically doing the same work as what's 
> done on line 3410 in the late failure case -- I wonder if this should 
> actually be tied to the `FailImmediatelyOnInvalidExpr` parameter instead of 
> having its own parameter?
They could be unified in the current version, but I don't think there is an 
actual relation between them.


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

https://reviews.llvm.org/D114439

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114439: [Annotation] Allow parameter pack expansions in annotate attribute

[Steffen Larsen via Phabricator via cfe-commits]

steffenlarsen updated this revision to Diff 403187.
steffenlarsen marked an inline comment as done.
steffenlarsen added a comment.

Removing top-level `const` and adjusting style.


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

https://reviews.llvm.org/D114439

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/include/clang/Parse/Parser.h
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Parse/ParseExpr.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/Parser/cxx0x-attributes.cpp
  clang/test/SemaTemplate/attributes.cpp
  clang/utils/TableGen/ClangAttrEmitter.cpp

Index: clang/utils/TableGen/ClangAttrEmitter.cpp
===
--- clang/utils/TableGen/ClangAttrEmitter.cpp
+++ clang/utils/TableGen/ClangAttrEmitter.cpp
@@ -1164,10 +1164,12 @@
   };
 
   class VariadicExprArgument : public VariadicArgument {
+bool AllowPack = false;
+
   public:
 VariadicExprArgument(const Record &Arg, StringRef Attr)
-  : VariadicArgument(Arg, Attr, "Expr *")
-{}
+: VariadicArgument(Arg, Attr, "Expr *"),
+  AllowPack(Arg.getValueAsBit("AllowPack")) {}
 
 void writeASTVisitorTraversal(raw_ostream &OS) const override {
   OS << "  {\n";
@@ -2264,6 +2266,47 @@
   OS << "#endif // CLANG_ATTR_THIS_ISA_IDENTIFIER_ARG_LIST\n\n";
 }
 
+static void emitClangAttrNumArgs(RecordKeeper &Records, raw_ostream &OS) {
+  OS << "#if defined(CLANG_ATTR_NUM_ARGS)\n";
+  std::vector Attrs = Records.getAllDerivedDefinitions("Attr");
+  for (const auto *A : Attrs) {
+std::vector Args = A->getValueAsListOfDefs("Args");
+forEachUniqueSpelling(*A, [&](const FlattenedSpelling &S) {
+  OS << ".Case(\"" << S.name() << "\", " << Args.size() << ")\n";
+});
+  }
+  OS << "#endif // CLANG_ATTR_NUM_ARGS\n\n";
+}
+
+static bool argSupportsParmPack(const Record *Arg) {
+  return !Arg->getSuperClasses().empty() &&
+ llvm::StringSwitch(
+ Arg->getSuperClasses().back().first->getName())
+ .Case("VariadicExprArgument", true)
+ .Default(false) &&
+ Arg->getValueAsBit("AllowPack");
+}
+
+static void emitClangAttrLastArgParmPackSupport(RecordKeeper &Records,
+raw_ostream &OS) {
+  OS << "#if defined(CLANG_ATTR_LAST_ARG_PARM_PACK_SUPPORT)\n";
+  std::vector Attrs = Records.getAllDerivedDefinitions("Attr");
+  for (const auto *A : Attrs) {
+std::vector Args = A->getValueAsListOfDefs("Args");
+bool LastArgSupportsParmPack =
+!Args.empty() && argSupportsParmPack(Args.back());
+forEachUniqueSpelling(*A, [&](const FlattenedSpelling &S) {
+  OS << ".Case(\"" << S.name() << "\", " << LastArgSupportsParmPack
+ << ")\n";
+});
+  }
+  OS << "#endif // CLANG_ATTR_LAST_ARG_PARM_PACK_SUPPORT\n\n";
+}
+
+static bool isArgVariadic(const Record &R, StringRef AttrName) {
+  return createArgument(R, AttrName)->isVariadic();
+}
+
 static void emitAttributes(RecordKeeper &Records, raw_ostream &OS,
bool Header) {
   std::vector Attrs = Records.getAllDerivedDefinitions("Attr");
@@ -2320,6 +2363,18 @@
 std::vector> Args;
 Args.reserve(ArgRecords.size());
 
+if (!ArgRecords.empty()) {
+  const bool LastArgSupportsPack = argSupportsParmPack(ArgRecords.back());
+  for (size_t I = 0; I < ArgRecords.size() - 1; ++I) {
+assert((!LastArgSupportsPack ||
+!isArgVariadic(*ArgRecords[I], R.getName())) &&
+   "Attributes supporting packs can only have the last "
+   "argument as variadic");
+assert(!argSupportsParmPack(ArgRecords[I]) &&
+   "Only the last argument can allow packs");
+  }
+}
+
 bool HasOptArg = false;
 bool HasFakeArg = false;
 for (const auto *ArgRecord : ArgRecords) {
@@ -3382,10 +3437,6 @@
   }
 }
 
-static bool isArgVariadic(const Record &R, StringRef AttrName) {
-  return createArgument(R, AttrName)->isVariadic();
-}
-
 static void emitArgInfo(const Record &R, raw_ostream &OS) {
   // This function will count the number of arguments specified for the
   // attribute and emit the number of required arguments followed by the
@@ -4219,6 +4270,8 @@
   emitClangAttrIdentifierArgList(Records, OS);
   emitClangAttrVariadicIdentifierArgList(Records, OS);
   emitClangAttrThisIsaIdentifierArgList(Records, OS);
+  emitClangAttrNumArgs(Records, OS);
+  emitClangAttrLastArgParmPackSupport(Records, OS);
   emitClangAttrTypeArgList(Records, OS);
   emitClangAttrLateParsedList(Records, OS);
 }
Index: clang/test/SemaTemplate/attributes.cpp
===
--- clang/test/SemaTemplate/attributes.cpp
+++ clang/test/SemaTemplate/attributes.cpp
@@ -64,6 +64,23 @@
 template [[clang::annotate("ANNOTATE_FOO"), clang::annotate("ANNOTATE_BAR")]] void HasAnnotations();
 void UseAnnotations() { HasAnnot

[PATCH] D114439: [Annotation] Allow parameter pack expansions in annotate attribute

[Steffen Larsen via Phabricator via cfe-commits]

steffenlarsen marked 2 inline comments as done.
steffenlarsen added inline comments.



Comment at: clang/lib/Parse/ParseDecl.cpp:497
+// Pack expansion is only allowed in the variadic argument at the end.
+const size_t attrNumArgs = attributeNumberOfArguments(*AttrName);
+if (ArgExprs.size() + I + 1 < attrNumArgs) {

aaron.ballman wrote:
> aaron.ballman wrote:
> > Coding style nits.
> We don't typically use top-level `const`, so that should go as well.
Completely missed that. My bad!



Comment at: clang/lib/Parse/ParseExpr.cpp:3374-3375
 
+if (EarlyTypoCorrection)
+  Expr = Actions.CorrectDelayedTyposInExpr(Expr);
+

aaron.ballman wrote:
> steffenlarsen wrote:
> > aaron.ballman wrote:
> > > steffenlarsen wrote:
> > > > aaron.ballman wrote:
> > > > > Not that I think this is a bad change, but it seems unrelated to the 
> > > > > patch. Can you explain why you made the change?
> > > > Admittedly I am not sure why it is needed, but in the previous version 
> > > > of the parsing for attribute parameters it does the typo-correction 
> > > > immediately after parsing the expression and if this isn't added a 
> > > > handful of tests fail.
> > > Ah, thanks for the info! So this is basically doing the same work as 
> > > what's done on line 3410 in the late failure case -- I wonder if this 
> > > should actually be tied to the `FailImmediatelyOnInvalidExpr` parameter 
> > > instead of having its own parameter?
> > They could be unified in the current version, but I don't think there is an 
> > actual relation between them.
> Do we fail tests if this isn't conditionalized at all (we always do the early 
> typo correction)? (I'd like to avoid adding the new parameter because I'm not 
> certain how a caller would determine whether they do or do not want that 
> behavior. If we need the parameter, then so be it, but it seems like this is 
> something generally useful.)
11 tests fail if we don't make it conditional. The most telling one is probably 
`SemaCXX/typo-correction-delayed.cpp` which will have more errors if we do typo 
correction early, so it seems that the delay is intentional. 



Comment at: clang/test/Parser/cxx0x-attributes.cpp:262
 
-template void variadic() {
+template  void variadic() {
   void bar [[noreturn...]] (); // expected-error {{attribute 'noreturn' cannot 
be used as an attribute pack}}

aaron.ballman wrote:
> You can back out the whitespace changes for an ever-so-slightly smaller 
> patch. :-D
Absolutely. Clang-format was very insistent, but since it isn't added by this 
patch I shouldn't worry. 😄 



Comment at: clang/test/Parser/cxx0x-attributes.cpp:276-277
+  void foz [[clang::annotate("E", 1, 2, 3, Is...)]] ();
+  void fiz [[clang::annotate("F", Is..., 1, 2, 3)]] ();
+  void fir [[clang::annotate("G", 1, Is..., 2, 3)]] ();
+}

aaron.ballman wrote:
> Shouldn't these cases also give the `// expected-error {{pack expansion in 
> 'annotate' may only be applied to the last argument}}` diagnostic?
These should preferably pass. They are part of the "last" argument as 
`annotate` has one non-variadic argument and one variadic argument. I agree 
though that the error message is a little vague. Originally the error did 
specify that it had to be "argument %1 or later" to try and convey this intent, 
but maybe you have an idea for a clearer error message?


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

https://reviews.llvm.org/D114439

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114439: [Annotation] Allow parameter pack expansions and initializer lists in annotate attribute

[Steffen Larsen via Phabricator via cfe-commits]

steffenlarsen updated this revision to Diff 403578.
steffenlarsen retitled this revision from "[Annotation] Allow parameter pack 
expansions in annotate attribute" to "[Annotation] Allow parameter pack 
expansions and initializer lists in annotate attribute".
steffenlarsen edited the summary of this revision.

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

https://reviews.llvm.org/D114439

Files:
  clang/include/clang/AST/Attr.h
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/include/clang/Parse/Parser.h
  clang/include/clang/Sema/Sema.h
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Parse/ParseExpr.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/Parser/cxx0x-attributes.cpp
  clang/test/SemaTemplate/attributes.cpp
  clang/utils/TableGen/ClangAttrEmitter.cpp

Index: clang/utils/TableGen/ClangAttrEmitter.cpp
===
--- clang/utils/TableGen/ClangAttrEmitter.cpp
+++ clang/utils/TableGen/ClangAttrEmitter.cpp
@@ -2138,6 +2138,11 @@
   }
 }
 
+static bool isTypeArgument(const Record *Arg) {
+  return !Arg->getSuperClasses().empty() &&
+ Arg->getSuperClasses().back().first->getName() == "TypeArgument";
+}
+
 /// Emits the first-argument-is-type property for attributes.
 static void emitClangAttrTypeArgList(RecordKeeper &Records, raw_ostream &OS) {
   OS << "#if defined(CLANG_ATTR_TYPE_ARG_LIST)\n";
@@ -2149,7 +2154,7 @@
 if (Args.empty())
   continue;
 
-if (Args[0]->getSuperClasses().back().first->getName() != "TypeArgument")
+if (!isTypeArgument(Args[0]))
   continue;
 
 // All these spellings take a single type argument.
@@ -2179,7 +2184,7 @@
   OS << "#endif // CLANG_ATTR_ARG_CONTEXT_LIST\n\n";
 }
 
-static bool isIdentifierArgument(Record *Arg) {
+static bool isIdentifierArgument(const Record *Arg) {
   return !Arg->getSuperClasses().empty() &&
 llvm::StringSwitch(Arg->getSuperClasses().back().first->getName())
 .Case("IdentifierArgument", true)
@@ -2188,7 +2193,7 @@
 .Default(false);
 }
 
-static bool isVariadicIdentifierArgument(Record *Arg) {
+static bool isVariadicIdentifierArgument(const Record *Arg) {
   return !Arg->getSuperClasses().empty() &&
  llvm::StringSwitch(
  Arg->getSuperClasses().back().first->getName())
@@ -2264,6 +2269,26 @@
   OS << "#endif // CLANG_ATTR_THIS_ISA_IDENTIFIER_ARG_LIST\n\n";
 }
 
+static void emitClangAttrAcceptsExprPack(RecordKeeper &Records,
+ raw_ostream &OS) {
+  OS << "#if defined(CLANG_ATTR_ACCEPTS_EXPR_PACK)\n";
+  ParsedAttrMap Attrs = getParsedAttrList(Records);
+  for (const auto &I : Attrs) {
+const Record &Attr = *I.second;
+
+if (!Attr.getValueAsBit("AcceptsExprPack"))
+  continue;
+
+// All these spellings take are parsed unevaluated.
+forEachUniqueSpelling(Attr, [&](const FlattenedSpelling &S) {
+  OS << ".Case(\"" << S.name() << "\", "
+ << "true"
+ << ")\n";
+});
+  }
+  OS << "#endif // CLANG_ATTR_ACCEPTS_EXPR_PACK\n\n";
+}
+
 static void emitAttributes(RecordKeeper &Records, raw_ostream &OS,
bool Header) {
   std::vector Attrs = Records.getAllDerivedDefinitions("Attr");
@@ -2320,6 +2345,17 @@
 std::vector> Args;
 Args.reserve(ArgRecords.size());
 
+bool AttrAcceptsExprPack = Attr->getValueAsBit("AcceptsExprPack");
+assert((!AttrAcceptsExprPack ||
+std::none_of(ArgRecords.begin(), ArgRecords.end(),
+ [&](const Record *ArgR) {
+   return isIdentifierArgument(ArgR) ||
+  isVariadicIdentifierArgument(ArgR) ||
+  isTypeArgument(ArgR);
+ })) &&
+   "Attributes accepting packs cannot also have identifier or type "
+   "arguments.");
+
 bool HasOptArg = false;
 bool HasFakeArg = false;
 for (const auto *ArgRecord : ArgRecords) {
@@ -2611,6 +2647,7 @@
   OS << "  A->Inherited = Inherited;\n";
   OS << "  A->IsPackExpansion = IsPackExpansion;\n";
   OS << "  A->setImplicit(Implicit);\n";
+  OS << "  A->setArgsDelayed(ArgsDelayed);\n";
   OS << "  return A;\n}\n\n";
 
   writePrettyPrintFunction(R, Args, OS);
@@ -2936,6 +2973,7 @@
   OS << "bool isInherited = Record.readInt();\n";
 OS << "bool isImplicit = Record.readInt();\n";
 OS << "bool isPackExpansion = Record.readInt();\n";
+OS << "bool areArgsDelayed = Record.readInt();\n";
 ArgRecords = R.getValueAsListOfDefs("Args");
 Args.clear();
 for (const auto *Arg : ArgRecords) {
@@ -2952,6 +2990,7 @@
   OS << "cast(New)->setInherited(isInherited);\n";
 OS << "New->setImplicit(isImplicit);\n";
 OS << "New->setPackExpansion(isPackExpansion);\n";
+OS << "New->setArgsDelayed(areArgsDelaye

[PATCH] D114439: [Annotation] Allow parameter pack expansions and initializer lists in annotate attribute

[Steffen Larsen via Phabricator via cfe-commits]

steffenlarsen added a comment.

Upon offline sync with @aaron.ballman and @erichkeane  I have changed the 
strategy of this patch to lift the restriction of pack expansion not spanning 
argument boundaries. This is implemented in `clang::annotate` by delaying 
arguments to after template-instantiation if any of the arguments are value 
dependent.


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

https://reviews.llvm.org/D114439

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114439: [Annotation] Allow parameter pack expansions and initializer lists in annotate attribute

[Steffen Larsen via Phabricator via cfe-commits]

steffenlarsen updated this revision to Diff 404440.
steffenlarsen edited the summary of this revision.

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

https://reviews.llvm.org/D114439

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/include/clang/Parse/Parser.h
  clang/include/clang/Sema/Sema.h
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Parse/ParseExpr.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/Parser/cxx0x-attributes.cpp
  clang/test/SemaTemplate/attributes.cpp
  clang/utils/TableGen/ClangAttrEmitter.cpp

Index: clang/utils/TableGen/ClangAttrEmitter.cpp
===
--- clang/utils/TableGen/ClangAttrEmitter.cpp
+++ clang/utils/TableGen/ClangAttrEmitter.cpp
@@ -202,9 +202,9 @@
 bool Fake;
 
   public:
-Argument(const Record &Arg, StringRef Attr)
-: lowerName(std::string(Arg.getValueAsString("Name"))),
-  upperName(lowerName), attrName(Attr), isOpt(false), Fake(false) {
+Argument(StringRef Arg, StringRef Attr)
+: lowerName(std::string(Arg)), upperName(lowerName), attrName(Attr),
+  isOpt(false), Fake(false) {
   if (!lowerName.empty()) {
 lowerName[0] = std::tolower(lowerName[0]);
 upperName[0] = std::toupper(upperName[0]);
@@ -215,6 +215,8 @@
   if (lowerName == "interface")
 lowerName = "interface_";
 }
+Argument(const Record &Arg, StringRef Attr)
+: Argument(Arg.getValueAsString("Name"), Attr) {}
 virtual ~Argument() = default;
 
 StringRef getLowerName() const { return lowerName; }
@@ -666,6 +668,11 @@
   ArgName(getLowerName().str() + "_"), ArgSizeName(ArgName + "Size"),
   RangeName(std::string(getLowerName())) {}
 
+VariadicArgument(StringRef Arg, StringRef Attr, std::string T)
+: Argument(Arg, Attr), Type(std::move(T)),
+  ArgName(getLowerName().str() + "_"), ArgSizeName(ArgName + "Size"),
+  RangeName(std::string(getLowerName())) {}
+
 const std::string &getType() const { return Type; }
 const std::string &getArgName() const { return ArgName; }
 const std::string &getArgSizeName() const { return ArgSizeName; }
@@ -688,6 +695,18 @@
  << "); }\n";
 }
 
+void writeSetter(raw_ostream &OS) const {
+  OS << "  void set" << getUpperName() << "(ASTContext &Ctx, ";
+  writeCtorParameters(OS);
+  OS << ") {\n";
+  OS << "" << ArgSizeName << " = " << getUpperName() << "Size;\n";
+  OS << "" << ArgName << " = new (Ctx, 16) " << getType() << "["
+ << ArgSizeName << "];\n";
+  OS << "  ";
+  writeCtorBody(OS);
+  OS << "  }\n";
+}
+
 void writeCloneArgs(raw_ostream &OS) const override {
   OS << ArgName << ", " << ArgSizeName;
 }
@@ -1169,6 +1188,9 @@
   : VariadicArgument(Arg, Attr, "Expr *")
 {}
 
+VariadicExprArgument(StringRef ArgName, StringRef Attr)
+: VariadicArgument(ArgName, Attr, "Expr *") {}
+
 void writeASTVisitorTraversal(raw_ostream &OS) const override {
   OS << "  {\n";
   OS << "" << getType() << " *I = A->" << getLowerName()
@@ -2138,6 +2160,11 @@
   }
 }
 
+static bool isTypeArgument(const Record *Arg) {
+  return !Arg->getSuperClasses().empty() &&
+ Arg->getSuperClasses().back().first->getName() == "TypeArgument";
+}
+
 /// Emits the first-argument-is-type property for attributes.
 static void emitClangAttrTypeArgList(RecordKeeper &Records, raw_ostream &OS) {
   OS << "#if defined(CLANG_ATTR_TYPE_ARG_LIST)\n";
@@ -2149,7 +2176,7 @@
 if (Args.empty())
   continue;
 
-if (Args[0]->getSuperClasses().back().first->getName() != "TypeArgument")
+if (!isTypeArgument(Args[0]))
   continue;
 
 // All these spellings take a single type argument.
@@ -2179,7 +2206,7 @@
   OS << "#endif // CLANG_ATTR_ARG_CONTEXT_LIST\n\n";
 }
 
-static bool isIdentifierArgument(Record *Arg) {
+static bool isIdentifierArgument(const Record *Arg) {
   return !Arg->getSuperClasses().empty() &&
 llvm::StringSwitch(Arg->getSuperClasses().back().first->getName())
 .Case("IdentifierArgument", true)
@@ -2188,7 +2215,7 @@
 .Default(false);
 }
 
-static bool isVariadicIdentifierArgument(Record *Arg) {
+static bool isVariadicIdentifierArgument(const Record *Arg) {
   return !Arg->getSuperClasses().empty() &&
  llvm::StringSwitch(
  Arg->getSuperClasses().back().first->getName())
@@ -2264,6 +2291,26 @@
   OS << "#endif // CLANG_ATTR_THIS_ISA_IDENTIFIER_ARG_LIST\n\n";
 }
 
+static void emitClangAttrAcceptsExprPack(RecordKeeper &Records,
+ raw_ostream &OS) {
+  OS << "#if defined(CLANG_ATTR_ACCEPTS_EXPR_PACK)\n";
+  ParsedAttrMap Attrs = getParsedAttrList(Records);
+  for (const auto &I : Attrs) {
+const Record &Attr = *I.second;
+
+if (!Attr.getValueAsBit("AcceptsExprPack")

[PATCH] D114439: [Annotation] Allow parameter pack expansions and initializer lists in annotate attribute

[Steffen Larsen via Phabricator via cfe-commits]

steffenlarsen marked 7 inline comments as done and an inline comment as not 
done.
steffenlarsen added inline comments.



Comment at: clang/include/clang/AST/Attr.h:51
   unsigned Implicit : 1;
+  unsigned ArgsDelayed : 1;
   // FIXME: These are properties of the attribute kind, not state for this

erichkeane wrote:
> What is this supposed to be for?  We should be able to tell if we weren't 
> able to instantiate the expressions based on whether the expr-list has stuff 
> in it, and if it is dependent.
You're right, it is redundant. I have removed it.



Comment at: clang/include/clang/Basic/Attr.td:208
 class VariadicUnsignedArgument : Argument;
-class VariadicExprArgument : Argument;
+class VariadicExprArgument : Argument;
 class VariadicStringArgument : Argument;

aaron.ballman wrote:
> erichkeane wrote:
> > I'm pretty against this 'fake', we should be putting this into the TableGen 
> > for every attribute that has AcceptsExprPack.
> Yeah, Fake is intended to be used for "internal details" and means that we 
> won't pretty print out that data from the AST node. These arguments aren't 
> really fake in that way -- they're things the user wrote in their source code.
I see. I have changed it to automatically generate a variadic expression 
"argument" named `DelayedArgs` if an attribute has `AcceptsExprPack` set. 



Comment at: clang/lib/Parse/ParseDecl.cpp:421-424
+  // Parse variadic identifier arg. This can either consume identifiers or
+  // expressions.
+  // FIXME: Variadic identifier args do not currently support parameter
+  //packs.

aaron.ballman wrote:
> (Might need to re-flow comments to 80 col.) I don't think this is a FIXME so 
> much as a "this just doesn't work like that" situation.
I think it makes sense to have it be a FIXME because in theory it could be 
possible to have expression parameter packs expanded in an identifier list as 
it accepts expressions. I have reworded it slightly. Do you think this is 
better?



Comment at: clang/lib/Parse/ParseDecl.cpp:460
+  CommaLocsTy CommaLocs;
+  ExprVector ParsedExprs;
+  if (ParseExpressionList(ParsedExprs, CommaLocs,

erichkeane wrote:
> So this all doesn't seem right either.  The algorithm here is essentially:
> 
> if (AcceptsArgPack) {
>   ParseExpressionList(); // plus error handling
> 
> //  Loop through args to see what matches correctly, if the 
> non-VariadicExprList arguments are non-dependent, fill those in, and create 
> as normal, else keep in the materialized collection.
> }
I am not convinced this is the right place for that. These changes are simply 
to allow the additional parsing of packs and similar. It does not care much for 
the different arguments of the attribute, beyond whether or not it accepts 
types or identifiers as well (see the other cases) so having it make decisions 
about how the attributes should populate their arguments seems out of line with 
the current structure.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4189
 
+void Sema::AddAnnotationAttrWithDelayedArgs(
+Decl *D, const AttributeCommonInfo &CI,

erichkeane wrote:
> I think this behavior (adding an attribute with a dependent expression list) 
> should be happening automatically.
> 
> In reality, whatever is doing the parsing or 
> potentially-partial-instantiation should be checking the 'StringLiteral' 
> value as a part of how it would without this being dependent.
> 
> That is: In the annotate case, the ONLY argument that we actually care about 
> being dependent or not is the 'first' one.  When we do the 
> parsing/instantiation, we need to detect if the non-variadic parameters can 
> be 'filled', then we can put the rest into the VariadicExpr list.
I have changed it so it will only delay the arguments if the the first argument 
is dependent. Is this in line with what you were thinking? Since we still need 
to create the attribute I am not sure how we would do this automatically.



Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:196
+SmallVector InstantiatedArgs;
+if (S.SubstExprs(ArrayRef{Attr->delayedArgs_begin(),
+  Attr->delayedArgs_end()},

erichkeane wrote:
> So I don't think this is the right approach (teaching SemaDeclAttr about the 
> delayed args).  After substitution here, we should be using these to fill in 
> the 'normal' arguments, then just passing these to the normal 
> 'handleAnnotateAttr'.
With the new structure I have simplified this part a little. SemaDeclAttr still 
knows about the delayed args but solely to create the attribute, while the 
logic for creating the attribute with the "normal" arguments has been separated 
and made part of Sema so it is reachable from this function.



Comment at: clang/test/Parser/cxx0x-attribut

[PATCH] D114439: [Annotation] Allow parameter pack expansions and initializer lists in annotate attribute

[Steffen Larsen via Phabricator via cfe-commits]

steffenlarsen marked 2 inline comments as done.
steffenlarsen added inline comments.



Comment at: clang/lib/Parse/ParseDecl.cpp:386
   ArgsVector ArgExprs;
   if (Tok.is(tok::identifier)) {
 // If this attribute wants an 'identifier' argument, make it so.

erichkeane wrote:
> So does this mean that our pack-parsing code is allowing identifiers?  I 
> really expected this patch to enforce via the clang-attr-emitter that ONLY 
> the 'expr' types were allowed.  So you'd have:
> 
> if (attributeAcceptsExprPack(AttrName)) {
> ... 
> } else if (Tok.is(tok::identifier)) {
> ...
> }...
This will only consume the identifier if the argument is an identifier argument 
(variadic or non-variadic). As you say, clang-attr-emitter will make sure that 
no attribute with `AcceptsExprPack` has any identifier arguments, so if there 
is an identifier here it will not be able to consume it and then it will 
continue on to try and parse it as an expression and fail.

That said, the second part of the following parsing branches should probably 
also check `attributeHasIdentifierArg(*AttrName)`, so I will add that shortly.


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

https://reviews.llvm.org/D114439

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114439: [Annotation] Allow parameter pack expansions and initializer lists in annotate attribute

[Steffen Larsen via Phabricator via cfe-commits]

steffenlarsen added inline comments.



Comment at: clang/lib/Parse/ParseDecl.cpp:386
   ArgsVector ArgExprs;
   if (Tok.is(tok::identifier)) {
 // If this attribute wants an 'identifier' argument, make it so.

steffenlarsen wrote:
> erichkeane wrote:
> > So does this mean that our pack-parsing code is allowing identifiers?  I 
> > really expected this patch to enforce via the clang-attr-emitter that ONLY 
> > the 'expr' types were allowed.  So you'd have:
> > 
> > if (attributeAcceptsExprPack(AttrName)) {
> > ... 
> > } else if (Tok.is(tok::identifier)) {
> > ...
> > }...
> This will only consume the identifier if the argument is an identifier 
> argument (variadic or non-variadic). As you say, clang-attr-emitter will make 
> sure that no attribute with `AcceptsExprPack` has any identifier arguments, 
> so if there is an identifier here it will not be able to consume it and then 
> it will continue on to try and parse it as an expression and fail.
> 
> That said, the second part of the following parsing branches should probably 
> also check `attributeHasIdentifierArg(*AttrName)`, so I will add that shortly.
> That said, the second part of the following parsing branches should probably 
> also check `attributeHasIdentifierArg(*AttrName)`, so I will add that shortly.

Correction: Parsing any identifiers after this assumes there is a variadic 
identifier argument. Effectively it assumes that all non-variadic identifiers 
in an attribute are at the start of an attribute arguments, in which case 
they'll be parsed here. This is a little lackluster as it could simply allow 
identifiers in other places if there are variadic identifier arguments in the 
attribute, but I don't think fixing it should be in the scope of this patch.


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

https://reviews.llvm.org/D114439

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114439: [Annotation] Allow parameter pack expansions and initializer lists in annotate attribute

[Steffen Larsen via Phabricator via cfe-commits]

steffenlarsen updated this revision to Diff 404526.
steffenlarsen added a comment.

Removed unnecessary test for arguments.


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

https://reviews.llvm.org/D114439

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/include/clang/Parse/Parser.h
  clang/include/clang/Sema/Sema.h
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Parse/ParseExpr.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/Parser/cxx0x-attributes.cpp
  clang/test/SemaTemplate/attributes.cpp
  clang/utils/TableGen/ClangAttrEmitter.cpp

Index: clang/utils/TableGen/ClangAttrEmitter.cpp
===
--- clang/utils/TableGen/ClangAttrEmitter.cpp
+++ clang/utils/TableGen/ClangAttrEmitter.cpp
@@ -202,9 +202,9 @@
 bool Fake;
 
   public:
-Argument(const Record &Arg, StringRef Attr)
-: lowerName(std::string(Arg.getValueAsString("Name"))),
-  upperName(lowerName), attrName(Attr), isOpt(false), Fake(false) {
+Argument(StringRef Arg, StringRef Attr)
+: lowerName(std::string(Arg)), upperName(lowerName), attrName(Attr),
+  isOpt(false), Fake(false) {
   if (!lowerName.empty()) {
 lowerName[0] = std::tolower(lowerName[0]);
 upperName[0] = std::toupper(upperName[0]);
@@ -215,6 +215,8 @@
   if (lowerName == "interface")
 lowerName = "interface_";
 }
+Argument(const Record &Arg, StringRef Attr)
+: Argument(Arg.getValueAsString("Name"), Attr) {}
 virtual ~Argument() = default;
 
 StringRef getLowerName() const { return lowerName; }
@@ -666,6 +668,11 @@
   ArgName(getLowerName().str() + "_"), ArgSizeName(ArgName + "Size"),
   RangeName(std::string(getLowerName())) {}
 
+VariadicArgument(StringRef Arg, StringRef Attr, std::string T)
+: Argument(Arg, Attr), Type(std::move(T)),
+  ArgName(getLowerName().str() + "_"), ArgSizeName(ArgName + "Size"),
+  RangeName(std::string(getLowerName())) {}
+
 const std::string &getType() const { return Type; }
 const std::string &getArgName() const { return ArgName; }
 const std::string &getArgSizeName() const { return ArgSizeName; }
@@ -688,6 +695,18 @@
  << "); }\n";
 }
 
+void writeSetter(raw_ostream &OS) const {
+  OS << "  void set" << getUpperName() << "(ASTContext &Ctx, ";
+  writeCtorParameters(OS);
+  OS << ") {\n";
+  OS << "" << ArgSizeName << " = " << getUpperName() << "Size;\n";
+  OS << "" << ArgName << " = new (Ctx, 16) " << getType() << "["
+ << ArgSizeName << "];\n";
+  OS << "  ";
+  writeCtorBody(OS);
+  OS << "  }\n";
+}
+
 void writeCloneArgs(raw_ostream &OS) const override {
   OS << ArgName << ", " << ArgSizeName;
 }
@@ -1169,6 +1188,9 @@
   : VariadicArgument(Arg, Attr, "Expr *")
 {}
 
+VariadicExprArgument(StringRef ArgName, StringRef Attr)
+: VariadicArgument(ArgName, Attr, "Expr *") {}
+
 void writeASTVisitorTraversal(raw_ostream &OS) const override {
   OS << "  {\n";
   OS << "" << getType() << " *I = A->" << getLowerName()
@@ -2138,6 +2160,11 @@
   }
 }
 
+static bool isTypeArgument(const Record *Arg) {
+  return !Arg->getSuperClasses().empty() &&
+ Arg->getSuperClasses().back().first->getName() == "TypeArgument";
+}
+
 /// Emits the first-argument-is-type property for attributes.
 static void emitClangAttrTypeArgList(RecordKeeper &Records, raw_ostream &OS) {
   OS << "#if defined(CLANG_ATTR_TYPE_ARG_LIST)\n";
@@ -2149,7 +2176,7 @@
 if (Args.empty())
   continue;
 
-if (Args[0]->getSuperClasses().back().first->getName() != "TypeArgument")
+if (!isTypeArgument(Args[0]))
   continue;
 
 // All these spellings take a single type argument.
@@ -2179,7 +2206,7 @@
   OS << "#endif // CLANG_ATTR_ARG_CONTEXT_LIST\n\n";
 }
 
-static bool isIdentifierArgument(Record *Arg) {
+static bool isIdentifierArgument(const Record *Arg) {
   return !Arg->getSuperClasses().empty() &&
 llvm::StringSwitch(Arg->getSuperClasses().back().first->getName())
 .Case("IdentifierArgument", true)
@@ -2188,7 +2215,7 @@
 .Default(false);
 }
 
-static bool isVariadicIdentifierArgument(Record *Arg) {
+static bool isVariadicIdentifierArgument(const Record *Arg) {
   return !Arg->getSuperClasses().empty() &&
  llvm::StringSwitch(
  Arg->getSuperClasses().back().first->getName())
@@ -2264,6 +2291,26 @@
   OS << "#endif // CLANG_ATTR_THIS_ISA_IDENTIFIER_ARG_LIST\n\n";
 }
 
+static void emitClangAttrAcceptsExprPack(RecordKeeper &Records,
+ raw_ostream &OS) {
+  OS << "#if defined(CLANG_ATTR_ACCEPTS_EXPR_PACK)\n";
+  ParsedAttrMap Attrs = getParsedAttrList(Records);
+  for (const auto &I : Attrs) {
+const Record &Attr = *I.second;
+
+if (!Attr.getValueAs

[PATCH] D114439: [Annotation] Allow parameter pack expansions and initializer lists in annotate attribute

[Steffen Larsen via Phabricator via cfe-commits]

steffenlarsen added inline comments.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4202
+  // If the first argument is value dependent we delay setting the arguments.
+  if (AllArgs.size() && AllArgs[0]->isValueDependent()) {
+auto *Attr = AnnotateAttr::CreateWithDelayedArgs(

erichkeane wrote:
> if AllArgs.size() == 0, this is an error case.  Annotate requires at least 1 
> parameter.
I removed the check but thinking about it again I think it would be better to 
reintroduce it and let `HandleAnnotateAttr` call `checkAtLeastNumArgs`. That 
way it will also complain about missing arguments after template instantiation, 
e.g. if a lone pack expansion argument evaluates to an empty expression list.


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

https://reviews.llvm.org/D114439

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114439: [Annotation] Allow parameter pack expansions and initializer lists in annotate attribute

[Steffen Larsen via Phabricator via cfe-commits]

steffenlarsen updated this revision to Diff 404548.
steffenlarsen added a comment.

Adds check and diagnostic for no arguments in annotate attribute.


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

https://reviews.llvm.org/D114439

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/include/clang/Parse/Parser.h
  clang/include/clang/Sema/Sema.h
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Parse/ParseExpr.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/Parser/cxx0x-attributes.cpp
  clang/test/Sema/annotate.c
  clang/test/SemaCXX/attr-annotate.cpp
  clang/test/SemaTemplate/attributes.cpp
  clang/utils/TableGen/ClangAttrEmitter.cpp

Index: clang/utils/TableGen/ClangAttrEmitter.cpp
===
--- clang/utils/TableGen/ClangAttrEmitter.cpp
+++ clang/utils/TableGen/ClangAttrEmitter.cpp
@@ -202,9 +202,9 @@
 bool Fake;
 
   public:
-Argument(const Record &Arg, StringRef Attr)
-: lowerName(std::string(Arg.getValueAsString("Name"))),
-  upperName(lowerName), attrName(Attr), isOpt(false), Fake(false) {
+Argument(StringRef Arg, StringRef Attr)
+: lowerName(std::string(Arg)), upperName(lowerName), attrName(Attr),
+  isOpt(false), Fake(false) {
   if (!lowerName.empty()) {
 lowerName[0] = std::tolower(lowerName[0]);
 upperName[0] = std::toupper(upperName[0]);
@@ -215,6 +215,8 @@
   if (lowerName == "interface")
 lowerName = "interface_";
 }
+Argument(const Record &Arg, StringRef Attr)
+: Argument(Arg.getValueAsString("Name"), Attr) {}
 virtual ~Argument() = default;
 
 StringRef getLowerName() const { return lowerName; }
@@ -666,6 +668,11 @@
   ArgName(getLowerName().str() + "_"), ArgSizeName(ArgName + "Size"),
   RangeName(std::string(getLowerName())) {}
 
+VariadicArgument(StringRef Arg, StringRef Attr, std::string T)
+: Argument(Arg, Attr), Type(std::move(T)),
+  ArgName(getLowerName().str() + "_"), ArgSizeName(ArgName + "Size"),
+  RangeName(std::string(getLowerName())) {}
+
 const std::string &getType() const { return Type; }
 const std::string &getArgName() const { return ArgName; }
 const std::string &getArgSizeName() const { return ArgSizeName; }
@@ -688,6 +695,18 @@
  << "); }\n";
 }
 
+void writeSetter(raw_ostream &OS) const {
+  OS << "  void set" << getUpperName() << "(ASTContext &Ctx, ";
+  writeCtorParameters(OS);
+  OS << ") {\n";
+  OS << "" << ArgSizeName << " = " << getUpperName() << "Size;\n";
+  OS << "" << ArgName << " = new (Ctx, 16) " << getType() << "["
+ << ArgSizeName << "];\n";
+  OS << "  ";
+  writeCtorBody(OS);
+  OS << "  }\n";
+}
+
 void writeCloneArgs(raw_ostream &OS) const override {
   OS << ArgName << ", " << ArgSizeName;
 }
@@ -1169,6 +1188,9 @@
   : VariadicArgument(Arg, Attr, "Expr *")
 {}
 
+VariadicExprArgument(StringRef ArgName, StringRef Attr)
+: VariadicArgument(ArgName, Attr, "Expr *") {}
+
 void writeASTVisitorTraversal(raw_ostream &OS) const override {
   OS << "  {\n";
   OS << "" << getType() << " *I = A->" << getLowerName()
@@ -2138,6 +2160,11 @@
   }
 }
 
+static bool isTypeArgument(const Record *Arg) {
+  return !Arg->getSuperClasses().empty() &&
+ Arg->getSuperClasses().back().first->getName() == "TypeArgument";
+}
+
 /// Emits the first-argument-is-type property for attributes.
 static void emitClangAttrTypeArgList(RecordKeeper &Records, raw_ostream &OS) {
   OS << "#if defined(CLANG_ATTR_TYPE_ARG_LIST)\n";
@@ -2149,7 +2176,7 @@
 if (Args.empty())
   continue;
 
-if (Args[0]->getSuperClasses().back().first->getName() != "TypeArgument")
+if (!isTypeArgument(Args[0]))
   continue;
 
 // All these spellings take a single type argument.
@@ -2179,7 +2206,7 @@
   OS << "#endif // CLANG_ATTR_ARG_CONTEXT_LIST\n\n";
 }
 
-static bool isIdentifierArgument(Record *Arg) {
+static bool isIdentifierArgument(const Record *Arg) {
   return !Arg->getSuperClasses().empty() &&
 llvm::StringSwitch(Arg->getSuperClasses().back().first->getName())
 .Case("IdentifierArgument", true)
@@ -2188,7 +2215,7 @@
 .Default(false);
 }
 
-static bool isVariadicIdentifierArgument(Record *Arg) {
+static bool isVariadicIdentifierArgument(const Record *Arg) {
   return !Arg->getSuperClasses().empty() &&
  llvm::StringSwitch(
  Arg->getSuperClasses().back().first->getName())
@@ -2264,6 +2291,26 @@
   OS << "#endif // CLANG_ATTR_THIS_ISA_IDENTIFIER_ARG_LIST\n\n";
 }
 
+static void emitClangAttrAcceptsExprPack(RecordKeeper &Records,
+ raw_ostream &OS) {
+  OS << "#if defined(CLANG_ATTR_ACCEPTS_EXPR_PACK)\n";
+  ParsedAttrMap Attrs = getParsedAttrList(Records);
+  

[PATCH] D114439: [Annotation] Allow parameter pack expansions and initializer lists in annotate attribute

[Steffen Larsen via Phabricator via cfe-commits]

steffenlarsen added inline comments.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4202
+  // If the first argument is value dependent we delay setting the arguments.
+  if (AllArgs.size() && AllArgs[0]->isValueDependent()) {
+auto *Attr = AnnotateAttr::CreateWithDelayedArgs(

steffenlarsen wrote:
> erichkeane wrote:
> > if AllArgs.size() == 0, this is an error case.  Annotate requires at least 
> > 1 parameter.
> I removed the check but thinking about it again I think it would be better to 
> reintroduce it and let `HandleAnnotateAttr` call `checkAtLeastNumArgs`. That 
> way it will also complain about missing arguments after template 
> instantiation, e.g. if a lone pack expansion argument evaluates to an empty 
> expression list.
I have reintroduced the check for `AllArgs.size()` here. This means if 
`AllArgs` is empty it will continue to `Sema::HandleAnnotateAttr` which will 
check that there are at least one element in `AllArgs` and fail otherwise with 
a useful diagnostic. This diagnostic will also be issued if an empty parameter 
pack is given and that is the only argument of the annotation. Note that 
`checkAtLeastNumArgs` is a member of `ParsedAttr` which isn't available in 
`Sema::HandleAnnotateAttr`, so it replicates the check. It could potentially be 
moved, but given the size I don't think it's necessary in this patch.

I also added some tests for checking that the diagnostic is issued in the cases 
(pack expansion and explicitly empty).

@erichkeane - What do you think?


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

https://reviews.llvm.org/D114439

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114439: [Annotation] Allow parameter pack expansions and initializer lists in annotate attribute

[Steffen Larsen via Phabricator via cfe-commits]

steffenlarsen added inline comments.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4202
+  // If the first argument is value dependent we delay setting the arguments.
+  if (AllArgs.size() && AllArgs[0]->isValueDependent()) {
+auto *Attr = AnnotateAttr::CreateWithDelayedArgs(

erichkeane wrote:
> steffenlarsen wrote:
> > steffenlarsen wrote:
> > > erichkeane wrote:
> > > > if AllArgs.size() == 0, this is an error case.  Annotate requires at 
> > > > least 1 parameter.
> > > I removed the check but thinking about it again I think it would be 
> > > better to reintroduce it and let `HandleAnnotateAttr` call 
> > > `checkAtLeastNumArgs`. That way it will also complain about missing 
> > > arguments after template instantiation, e.g. if a lone pack expansion 
> > > argument evaluates to an empty expression list.
> > I have reintroduced the check for `AllArgs.size()` here. This means if 
> > `AllArgs` is empty it will continue to `Sema::HandleAnnotateAttr` which 
> > will check that there are at least one element in `AllArgs` and fail 
> > otherwise with a useful diagnostic. This diagnostic will also be issued if 
> > an empty parameter pack is given and that is the only argument of the 
> > annotation. Note that `checkAtLeastNumArgs` is a member of `ParsedAttr` 
> > which isn't available in `Sema::HandleAnnotateAttr`, so it replicates the 
> > check. It could potentially be moved, but given the size I don't think it's 
> > necessary in this patch.
> > 
> > I also added some tests for checking that the diagnostic is issued in the 
> > cases (pack expansion and explicitly empty).
> > 
> > @erichkeane - What do you think?
> Hmm... I would expect this to diagnose even in the 'delayed args' case as 
> well.  Otherwise:
> 
> template
> [[annotate()]]
> 
> is not an error, right?
Both "delayed args" and immediate instantiation go through the diagnostics 
check. The check is intentionally in `Sema::HandleAnnotateAttr` as either this 
or template instantiation (if delayed args) goes through it.

```
template [[annotate()]] ...
```

would fail because it does not have any arguments which will be identified here 
as "delayed args" will be skipped. 

```
template [[annotate(Is...)]] ...
```

will fail once instantiated with no template arguments as the instantiating 
function will pass an empty list of expressions to `Sema::HandleAnnotationAttr` 
which will check and fail with the diagnostic. The latter is tested and I can 
add the former as a case while adding the other requested tests.


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

https://reviews.llvm.org/D114439

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114439: [Annotation] Allow parameter pack expansions and initializer lists in annotate attribute

[Steffen Larsen via Phabricator via cfe-commits]

steffenlarsen updated this revision to Diff 404889.
steffenlarsen added a comment.

Added tests for redeclarations and template specialization using 
`clang::annoate` with packs.


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

https://reviews.llvm.org/D114439

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/include/clang/Parse/Parser.h
  clang/include/clang/Sema/Sema.h
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Parse/ParseExpr.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/Parser/cxx0x-attributes.cpp
  clang/test/Sema/annotate.c
  clang/test/SemaCXX/attr-annotate.cpp
  clang/test/SemaTemplate/attributes.cpp
  clang/utils/TableGen/ClangAttrEmitter.cpp

Index: clang/utils/TableGen/ClangAttrEmitter.cpp
===
--- clang/utils/TableGen/ClangAttrEmitter.cpp
+++ clang/utils/TableGen/ClangAttrEmitter.cpp
@@ -202,9 +202,9 @@
 bool Fake;
 
   public:
-Argument(const Record &Arg, StringRef Attr)
-: lowerName(std::string(Arg.getValueAsString("Name"))),
-  upperName(lowerName), attrName(Attr), isOpt(false), Fake(false) {
+Argument(StringRef Arg, StringRef Attr)
+: lowerName(std::string(Arg)), upperName(lowerName), attrName(Attr),
+  isOpt(false), Fake(false) {
   if (!lowerName.empty()) {
 lowerName[0] = std::tolower(lowerName[0]);
 upperName[0] = std::toupper(upperName[0]);
@@ -215,6 +215,8 @@
   if (lowerName == "interface")
 lowerName = "interface_";
 }
+Argument(const Record &Arg, StringRef Attr)
+: Argument(Arg.getValueAsString("Name"), Attr) {}
 virtual ~Argument() = default;
 
 StringRef getLowerName() const { return lowerName; }
@@ -666,6 +668,11 @@
   ArgName(getLowerName().str() + "_"), ArgSizeName(ArgName + "Size"),
   RangeName(std::string(getLowerName())) {}
 
+VariadicArgument(StringRef Arg, StringRef Attr, std::string T)
+: Argument(Arg, Attr), Type(std::move(T)),
+  ArgName(getLowerName().str() + "_"), ArgSizeName(ArgName + "Size"),
+  RangeName(std::string(getLowerName())) {}
+
 const std::string &getType() const { return Type; }
 const std::string &getArgName() const { return ArgName; }
 const std::string &getArgSizeName() const { return ArgSizeName; }
@@ -688,6 +695,18 @@
  << "); }\n";
 }
 
+void writeSetter(raw_ostream &OS) const {
+  OS << "  void set" << getUpperName() << "(ASTContext &Ctx, ";
+  writeCtorParameters(OS);
+  OS << ") {\n";
+  OS << "" << ArgSizeName << " = " << getUpperName() << "Size;\n";
+  OS << "" << ArgName << " = new (Ctx, 16) " << getType() << "["
+ << ArgSizeName << "];\n";
+  OS << "  ";
+  writeCtorBody(OS);
+  OS << "  }\n";
+}
+
 void writeCloneArgs(raw_ostream &OS) const override {
   OS << ArgName << ", " << ArgSizeName;
 }
@@ -1169,6 +1188,9 @@
   : VariadicArgument(Arg, Attr, "Expr *")
 {}
 
+VariadicExprArgument(StringRef ArgName, StringRef Attr)
+: VariadicArgument(ArgName, Attr, "Expr *") {}
+
 void writeASTVisitorTraversal(raw_ostream &OS) const override {
   OS << "  {\n";
   OS << "" << getType() << " *I = A->" << getLowerName()
@@ -2138,6 +2160,11 @@
   }
 }
 
+static bool isTypeArgument(const Record *Arg) {
+  return !Arg->getSuperClasses().empty() &&
+ Arg->getSuperClasses().back().first->getName() == "TypeArgument";
+}
+
 /// Emits the first-argument-is-type property for attributes.
 static void emitClangAttrTypeArgList(RecordKeeper &Records, raw_ostream &OS) {
   OS << "#if defined(CLANG_ATTR_TYPE_ARG_LIST)\n";
@@ -2149,7 +2176,7 @@
 if (Args.empty())
   continue;
 
-if (Args[0]->getSuperClasses().back().first->getName() != "TypeArgument")
+if (!isTypeArgument(Args[0]))
   continue;
 
 // All these spellings take a single type argument.
@@ -2179,7 +2206,7 @@
   OS << "#endif // CLANG_ATTR_ARG_CONTEXT_LIST\n\n";
 }
 
-static bool isIdentifierArgument(Record *Arg) {
+static bool isIdentifierArgument(const Record *Arg) {
   return !Arg->getSuperClasses().empty() &&
 llvm::StringSwitch(Arg->getSuperClasses().back().first->getName())
 .Case("IdentifierArgument", true)
@@ -2188,7 +2215,7 @@
 .Default(false);
 }
 
-static bool isVariadicIdentifierArgument(Record *Arg) {
+static bool isVariadicIdentifierArgument(const Record *Arg) {
   return !Arg->getSuperClasses().empty() &&
  llvm::StringSwitch(
  Arg->getSuperClasses().back().first->getName())
@@ -2264,6 +2291,26 @@
   OS << "#endif // CLANG_ATTR_THIS_ISA_IDENTIFIER_ARG_LIST\n\n";
 }
 
+static void emitClangAttrAcceptsExprPack(RecordKeeper &Records,
+ raw_ostream &OS) {
+  OS << "#if defined(CLANG_ATTR_ACCEPTS_EXPR_PACK)\n";
+  ParsedAttrMap Attrs = ge

[PATCH] D114439: [Annotation] Allow parameter pack expansions and initializer lists in annotate attribute

[Steffen Larsen via Phabricator via cfe-commits]

steffenlarsen marked 3 inline comments as done.
steffenlarsen added inline comments.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4179
+  MutableArrayRef AllArgs) {
+  assert(AllArgs.size());
+

erichkeane wrote:
> Does this work with a partial specialization?  I'd like to see some tests 
> that ensure we work in that case (both where the partial specialization sets 
> the string literal correctly, and where it doesnt).
I have added such tests in `clang/test/SemaTemplate/attributes.cpp`. Let me 
know if they cover the scenario you were thinking about.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4202
+  // If the first argument is value dependent we delay setting the arguments.
+  if (AllArgs.size() && AllArgs[0]->isValueDependent()) {
+auto *Attr = AnnotateAttr::CreateWithDelayedArgs(

erichkeane wrote:
> steffenlarsen wrote:
> > erichkeane wrote:
> > > steffenlarsen wrote:
> > > > steffenlarsen wrote:
> > > > > erichkeane wrote:
> > > > > > if AllArgs.size() == 0, this is an error case.  Annotate requires 
> > > > > > at least 1 parameter.
> > > > > I removed the check but thinking about it again I think it would be 
> > > > > better to reintroduce it and let `HandleAnnotateAttr` call 
> > > > > `checkAtLeastNumArgs`. That way it will also complain about missing 
> > > > > arguments after template instantiation, e.g. if a lone pack expansion 
> > > > > argument evaluates to an empty expression list.
> > > > I have reintroduced the check for `AllArgs.size()` here. This means if 
> > > > `AllArgs` is empty it will continue to `Sema::HandleAnnotateAttr` which 
> > > > will check that there are at least one element in `AllArgs` and fail 
> > > > otherwise with a useful diagnostic. This diagnostic will also be issued 
> > > > if an empty parameter pack is given and that is the only argument of 
> > > > the annotation. Note that `checkAtLeastNumArgs` is a member of 
> > > > `ParsedAttr` which isn't available in `Sema::HandleAnnotateAttr`, so it 
> > > > replicates the check. It could potentially be moved, but given the size 
> > > > I don't think it's necessary in this patch.
> > > > 
> > > > I also added some tests for checking that the diagnostic is issued in 
> > > > the cases (pack expansion and explicitly empty).
> > > > 
> > > > @erichkeane - What do you think?
> > > Hmm... I would expect this to diagnose even in the 'delayed args' case as 
> > > well.  Otherwise:
> > > 
> > > template
> > > [[annotate()]]
> > > 
> > > is not an error, right?
> > Both "delayed args" and immediate instantiation go through the diagnostics 
> > check. The check is intentionally in `Sema::HandleAnnotateAttr` as either 
> > this or template instantiation (if delayed args) goes through it.
> > 
> > ```
> > template [[annotate()]] ...
> > ```
> > 
> > would fail because it does not have any arguments which will be identified 
> > here as "delayed args" will be skipped. 
> > 
> > ```
> > template [[annotate(Is...)]] ...
> > ```
> > 
> > will fail once instantiated with no template arguments as the instantiating 
> > function will pass an empty list of expressions to 
> > `Sema::HandleAnnotationAttr` which will check and fail with the diagnostic. 
> > The latter is tested and I can add the former as a case while adding the 
> > other requested tests.
> Can you make sure both of those have a test?  I missed that the 'no args' 
> case would go right to HandleAnnotateAttr.
For sure. It has been added.



Comment at: clang/test/Parser/cxx0x-attributes.cpp:276
+  void fiz [[clang::annotate("F", Is..., 1, 2, 3)]] ();
+  void fir [[clang::annotate("G", 1, Is..., 2, 3)]] ();
+}

steffenlarsen wrote:
> aaron.ballman wrote:
> > We should also be adding test coverage for things like redeclarations, 
> > partial specializations, etc to make sure that the behavior there is 
> > correct. Those would be Sema tests rather than Parser tests, however.
> Good point. I will add these tests ASAP.
I have added such tests in `clang/test/SemaTemplate/attributes.cpp`. Are these 
along the line of what you were thinking?


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

https://reviews.llvm.org/D114439

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114439: [Annotation] Allow parameter pack expansions in annotate attribute

[Steffen Larsen via Phabricator via cfe-commits]

steffenlarsen updated this revision to Diff 399334.
steffenlarsen added a comment.

I have made the changes suggested in my previous comment. This makes 
significantly more changes to the parsing of attribute arguments as the old 
path was needed for attributes that allow both expressions and types. It also 
required some new controlling arguments for `ParseExpressionList`.

Because these changes also allow intializer lists in attribute arguments I have 
added a test case showing that `clang::annotate` parses these but will not 
accept them.

@erichkeane & @aaron.ballman - Is this in line with what you were thinking?


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

https://reviews.llvm.org/D114439

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/include/clang/Parse/Parser.h
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Parse/ParseExpr.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/Parser/cxx0x-attributes.cpp
  clang/test/SemaTemplate/attributes.cpp
  clang/utils/TableGen/ClangAttrEmitter.cpp

Index: clang/utils/TableGen/ClangAttrEmitter.cpp
===
--- clang/utils/TableGen/ClangAttrEmitter.cpp
+++ clang/utils/TableGen/ClangAttrEmitter.cpp
@@ -1164,10 +1164,12 @@
   };
 
   class VariadicExprArgument : public VariadicArgument {
+bool AllowPack = false;
+
   public:
 VariadicExprArgument(const Record &Arg, StringRef Attr)
-  : VariadicArgument(Arg, Attr, "Expr *")
-{}
+: VariadicArgument(Arg, Attr, "Expr *"),
+  AllowPack(Arg.getValueAsBit("AllowPack")) {}
 
 void writeASTVisitorTraversal(raw_ostream &OS) const override {
   OS << "  {\n";
@@ -2264,6 +2266,47 @@
   OS << "#endif // CLANG_ATTR_THIS_ISA_IDENTIFIER_ARG_LIST\n\n";
 }
 
+static void emitClangAttrNumArgs(RecordKeeper &Records, raw_ostream &OS) {
+  OS << "#if defined(CLANG_ATTR_NUM_ARGS)\n";
+  std::vector Attrs = Records.getAllDerivedDefinitions("Attr");
+  for (const auto *A : Attrs) {
+std::vector Args = A->getValueAsListOfDefs("Args");
+forEachUniqueSpelling(*A, [&](const FlattenedSpelling &S) {
+  OS << ".Case(\"" << S.name() << "\", " << Args.size() << ")\n";
+});
+  }
+  OS << "#endif // CLANG_ATTR_NUM_ARGS\n\n";
+}
+
+static bool argSupportsParmPack(const Record *Arg) {
+  return !Arg->getSuperClasses().empty() &&
+ llvm::StringSwitch(
+ Arg->getSuperClasses().back().first->getName())
+ .Case("VariadicExprArgument", true)
+ .Default(false) &&
+ Arg->getValueAsBit("AllowPack");
+}
+
+static void emitClangAttrLastArgParmPackSupport(RecordKeeper &Records,
+raw_ostream &OS) {
+  OS << "#if defined(CLANG_ATTR_LAST_ARG_PARM_PACK_SUPPORT)\n";
+  std::vector Attrs = Records.getAllDerivedDefinitions("Attr");
+  for (const auto *A : Attrs) {
+std::vector Args = A->getValueAsListOfDefs("Args");
+bool LastArgSupportsParmPack =
+!Args.empty() && argSupportsParmPack(Args.back());
+forEachUniqueSpelling(*A, [&](const FlattenedSpelling &S) {
+  OS << ".Case(\"" << S.name() << "\", " << LastArgSupportsParmPack
+ << ")\n";
+});
+  }
+  OS << "#endif // CLANG_ATTR_LAST_ARG_PARM_PACK_SUPPORT\n\n";
+}
+
+static bool isArgVariadic(const Record &R, StringRef AttrName) {
+  return createArgument(R, AttrName)->isVariadic();
+}
+
 static void emitAttributes(RecordKeeper &Records, raw_ostream &OS,
bool Header) {
   std::vector Attrs = Records.getAllDerivedDefinitions("Attr");
@@ -2320,6 +2363,18 @@
 std::vector> Args;
 Args.reserve(ArgRecords.size());
 
+if (!ArgRecords.empty()) {
+  const bool LastArgSupportsPack = argSupportsParmPack(ArgRecords.back());
+  for (size_t I = 0; I < ArgRecords.size() - 1; ++I) {
+assert((!LastArgSupportsPack ||
+!isArgVariadic(*ArgRecords[I], R.getName())) &&
+   "Attributes supporting packs can only have the last "
+   "argument as variadic");
+assert(!argSupportsParmPack(ArgRecords[I]) &&
+   "Only the last argument can allow packs");
+  }
+}
+
 bool HasOptArg = false;
 bool HasFakeArg = false;
 for (const auto *ArgRecord : ArgRecords) {
@@ -3382,10 +3437,6 @@
   }
 }
 
-static bool isArgVariadic(const Record &R, StringRef AttrName) {
-  return createArgument(R, AttrName)->isVariadic();
-}
-
 static void emitArgInfo(const Record &R, raw_ostream &OS) {
   // This function will count the number of arguments specified for the
   // attribute and emit the number of required arguments followed by the
@@ -4219,6 +4270,8 @@
   emitClangAttrIdentifierArgList(Records, OS);
   emitClangAttrVariadicIdentifierArgList(Records, OS);
   emitClangAttrThisIsaIdentifierArgList(Records, OS);
+  emitClangAttrNumArgs(Records, OS);
+  emitClangAttrLastArgParmPackSupport(Records, OS);

[PATCH] D114439: [Annotation] Allow parameter pack expansions in annotate attribute

[Steffen Larsen via Phabricator via cfe-commits]

steffenlarsen added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:724
+  "attribute %0 does not support pack expansion in the last argument">;
+def err_attribute_parm_pack_last_argument_only : Error<
+  "pack expansion in attributes is restricted to only the last argument">;

erichkeane wrote:
> I don't really see why this is required?  I would think the 722 would be all 
> you would need.
Intention was to make a distinction between the two cases, since we had the 
granularity to do so. I.e. `clang::annotate` would never use 722 as it 
explicitly states that the attribute doesn't support pack expansion (in the 
last argument) which is untrue for `clang::annotate`, but if a user was to do 
pack expansion on the first argument of `clang::annotate` they would get this 
error telling them that the given argument cannot accept the pack expansion.



Comment at: clang/lib/Parse/ParseDecl.cpp:450
+  // Pack expansion is only allowed in the last attribute argument.
+  if (ArgExprs.size() + 1 < attributeNumberOfArguments(*AttrName)) {
+Diag(Tok.getLocation(),

erichkeane wrote:
> I don't think this should be diagnosed here, and I don't think it is 'right'. 
>  I think the ClangAttrEmitter should ensure that the VariadicExprArgument 
> needs to be the 'last' thing, but I think that REALLY means "supports a pack 
> anywhere inside of it".
> 
> See my test examples below, I don't think this parsing is sufficient for that.
That is also the intention here. All it checks is that we are in or beyond the 
last argument of the attribute. For example, `clang::annotate` will return 2 
from `attributeNumberOfArguments` as `VariadicExprArgument` only counts as a 
single argument. Thus, any pack expansion expressions parsed after the first 
will be accepted. I do agree though that the error message may be a little 
confusing for users.

I will add the suggested tests and rethink the diagnostics.


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

https://reviews.llvm.org/D114439

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114439: [Annotation] Allow parameter pack expansions in annotate attribute

[Steffen Larsen via Phabricator via cfe-commits]

steffenlarsen updated this revision to Diff 392340.
steffenlarsen added a comment.

The new revision does the following:

1. Revisits the diagnostics. Now parameter packs will cause the same error on 
attributes that do not support parameter packs in arguments, while attributes 
that support it will cause an error if parameter packs are used in the 
incorrect argument, specifying which argument should be the earliest for it.
2. Adds additional tests to ensure that variadic arguments supporting packs 
also allow pack expansion intermingled with other arguments.
3. Adds assertions to attribute generation to check that attributes only ever 
support packs in the last variadic argument and that there are no other 
variadic arguments if they do.


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

https://reviews.llvm.org/D114439

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/Parser/cxx0x-attributes.cpp
  clang/test/SemaTemplate/attributes.cpp
  clang/utils/TableGen/ClangAttrEmitter.cpp

Index: clang/utils/TableGen/ClangAttrEmitter.cpp
===
--- clang/utils/TableGen/ClangAttrEmitter.cpp
+++ clang/utils/TableGen/ClangAttrEmitter.cpp
@@ -1164,10 +1164,12 @@
   };
 
   class VariadicExprArgument : public VariadicArgument {
+bool AllowPack = false;
+
   public:
 VariadicExprArgument(const Record &Arg, StringRef Attr)
-  : VariadicArgument(Arg, Attr, "Expr *")
-{}
+: VariadicArgument(Arg, Attr, "Expr *"),
+  AllowPack(Arg.getValueAsBit("AllowPack")) {}
 
 void writeASTVisitorTraversal(raw_ostream &OS) const override {
   OS << "  {\n";
@@ -2264,6 +2266,47 @@
   OS << "#endif // CLANG_ATTR_THIS_ISA_IDENTIFIER_ARG_LIST\n\n";
 }
 
+static void emitClangAttrNumArgs(RecordKeeper &Records, raw_ostream &OS) {
+  OS << "#if defined(CLANG_ATTR_NUM_ARGS)\n";
+  std::vector Attrs = Records.getAllDerivedDefinitions("Attr");
+  for (const auto *A : Attrs) {
+std::vector Args = A->getValueAsListOfDefs("Args");
+forEachUniqueSpelling(*A, [&](const FlattenedSpelling &S) {
+  OS << ".Case(\"" << S.name() << "\", " << Args.size() << ")\n";
+});
+  }
+  OS << "#endif // CLANG_ATTR_NUM_ARGS\n\n";
+}
+
+static bool argSupportsParmPack(const Record *Arg) {
+  return !Arg->getSuperClasses().empty() &&
+ llvm::StringSwitch(
+ Arg->getSuperClasses().back().first->getName())
+ .Case("VariadicExprArgument", true)
+ .Default(false) &&
+ Arg->getValueAsBit("AllowPack");
+}
+
+static void emitClangAttrLastArgParmPackSupport(RecordKeeper &Records,
+raw_ostream &OS) {
+  OS << "#if defined(CLANG_ATTR_LAST_ARG_PARM_PACK_SUPPORT)\n";
+  std::vector Attrs = Records.getAllDerivedDefinitions("Attr");
+  for (const auto *A : Attrs) {
+std::vector Args = A->getValueAsListOfDefs("Args");
+bool LastArgSupportsParmPack =
+!Args.empty() && argSupportsParmPack(Args.back());
+forEachUniqueSpelling(*A, [&](const FlattenedSpelling &S) {
+  OS << ".Case(\"" << S.name() << "\", " << LastArgSupportsParmPack
+ << ")\n";
+});
+  }
+  OS << "#endif // CLANG_ATTR_LAST_ARG_PARM_PACK_SUPPORT\n\n";
+}
+
+static bool isArgVariadic(const Record &R, StringRef AttrName) {
+  return createArgument(R, AttrName)->isVariadic();
+}
+
 static void emitAttributes(RecordKeeper &Records, raw_ostream &OS,
bool Header) {
   std::vector Attrs = Records.getAllDerivedDefinitions("Attr");
@@ -2320,6 +2363,18 @@
 std::vector> Args;
 Args.reserve(ArgRecords.size());
 
+if (!ArgRecords.empty()) {
+  const bool LastArgSupportsPack = argSupportsParmPack(ArgRecords.back());
+  for (size_t I = 0; I < ArgRecords.size() - 1; ++I) {
+assert((!LastArgSupportsPack ||
+!isArgVariadic(*ArgRecords[I], R.getName())) &&
+   "Attributes supporting packs can only have the last "
+   "argument as variadic");
+assert(!argSupportsParmPack(ArgRecords[I]) &&
+   "Only the last argument can allow packs");
+  }
+}
+
 bool HasOptArg = false;
 bool HasFakeArg = false;
 for (const auto *ArgRecord : ArgRecords) {
@@ -3382,10 +3437,6 @@
   }
 }
 
-static bool isArgVariadic(const Record &R, StringRef AttrName) {
-  return createArgument(R, AttrName)->isVariadic();
-}
-
 static void emitArgInfo(const Record &R, raw_ostream &OS) {
   // This function will count the number of arguments specified for the
   // attribute and emit the number of required arguments followed by the
@@ -4219,6 +4270,8 @@
   emitClangAttrIdentifierArgList(Records, OS);
   emitClangAttrVariadicIdentifierArgList(Records, OS);
   emitClangAttrThisIsaIdentifierArgList(Records, OS);
+  emitClangAttrNumArgs(Records, O

[PATCH] D114439: [Annotation] Allow parameter pack expansions in annotate attribute

[Steffen Larsen via Phabricator via cfe-commits]

steffenlarsen added inline comments.



Comment at: clang/test/Parser/cxx0x-attributes.cpp:268
+  void faz [[clang::annotate("B", (Is + ...))]] (); // expected-warning {{pack 
fold expression is a C++17 extension}}
+  void foz [[clang::annotate("C", Is...)]] ();
 }

erichkeane wrote:
> what about:
> void foz [[clang::annotate("D", Is)]] ();
> 
> I would expect that to error.
> 
> Another test I'd like to see:
> 
> void foz[[clang::annotate("E", 1, 2, 3, Is...)]]
> 
> Also, I don't see why if THAT works, that:
> void foz[[clang::annotate("E", 1, Is..., 2, 3)]]
> 
> shouldn't be allowed as well.
> what about:
> void foz [[clang::annotate("D", Is)]] ();
>
> I would expect that to error.

So would I, but it seems that the parser and annotate is more than happy to 
consume this example, even in the current state of Clang: 
https://godbolt.org/z/rx64EKeeE
I am not sure as to why, but seeing as it is a preexisting bug I don't know if 
it makes sense to include it in the scope of this patch.

>Another test I'd like to see:
>
> void foz[[clang::annotate("E", 1, 2, 3, Is...)]]
> 
> Also, I don't see why if THAT works, that:
> void foz[[clang::annotate("E", 1, Is..., 2, 3)]]
> 
> shouldn't be allowed as well.

They seem to work as expected. I have added these cases.



Comment at: clang/utils/TableGen/ClangAttrEmitter.cpp:1166
 
   class VariadicExprArgument : public VariadicArgument {
+bool AllowPack = false;

erichkeane wrote:
> The rule of 'only the last argument is allowed to support a pack' should be 
> in the attribute emitter.
> The rule of 'only the last argument is allowed to support a pack' should be 
> in the attribute emitter.

I have added some assertions around the area where attributes are generate, as 
that carries more surrounding information. Is that approximately what you had 
in mind?


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

https://reviews.llvm.org/D114439

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114439: [Annotation] Allow parameter pack expansions in annotate attribute

[Steffen Larsen via Phabricator via cfe-commits]

steffenlarsen added inline comments.



Comment at: clang/lib/Parse/ParseDecl.cpp:447
 Actions.CorrectDelayedTyposInExpr(ParseAssignmentExpression()));
+
+if (Tok.is(tok::ellipsis)) {

erichkeane wrote:
> So I was thinking about this overnight... I wonder if this logic is inverted 
> here?  Aaron can better answer, but I wonder if we should be instead 
> detecting when we are on the 'last' parameter and it is one of these 
> `VariadicExprArgument` things (that accept a pack), then dropping the parser 
> into a loop of:
> 
> while (Tok.is(tok::comma)){
>   Tok.Consume();
>   ArgExpr = CorrectTypos(ParseAssignmentExpr(...));
> }
> // finally, consume the closing paren.
> 
> WDYT?
The parsing of attribute arguments is already this lenient. It does not 
actually care how many arguments the attribute says it takes (unless it takes a 
type and then it will only currently supply _one_ argument). It simply consumes 
as many expressions it can before reaching the end parenthesis, then leaves the 
attributes to consume them in `clang/lib/Sema/SemaDeclAttr.cpp`.
As a result we do not need any additional work here to handle variadic 
arguments, but it also means that we have to introduce the restrictions that we 
can only allow packs in the last argument and allow only that argument to be 
variadic, as otherwise we have no way of knowing when and where the packs pass 
between argument boundaries.


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

https://reviews.llvm.org/D114439

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114439: [Annotation] Allow parameter pack expansions in annotate attribute

[Steffen Larsen via Phabricator via cfe-commits]

steffenlarsen added inline comments.



Comment at: clang/lib/Parse/ParseDecl.cpp:447
 Actions.CorrectDelayedTyposInExpr(ParseAssignmentExpression()));
+
+if (Tok.is(tok::ellipsis)) {

erichkeane wrote:
> steffenlarsen wrote:
> > erichkeane wrote:
> > > So I was thinking about this overnight... I wonder if this logic is 
> > > inverted here?  Aaron can better answer, but I wonder if we should be 
> > > instead detecting when we are on the 'last' parameter and it is one of 
> > > these `VariadicExprArgument` things (that accept a pack), then dropping 
> > > the parser into a loop of:
> > > 
> > > while (Tok.is(tok::comma)){
> > >   Tok.Consume();
> > >   ArgExpr = CorrectTypos(ParseAssignmentExpr(...));
> > > }
> > > // finally, consume the closing paren.
> > > 
> > > WDYT?
> > The parsing of attribute arguments is already this lenient. It does not 
> > actually care how many arguments the attribute says it takes (unless it 
> > takes a type and then it will only currently supply _one_ argument). It 
> > simply consumes as many expressions it can before reaching the end 
> > parenthesis, then leaves the attributes to consume them in 
> > `clang/lib/Sema/SemaDeclAttr.cpp`.
> > As a result we do not need any additional work here to handle variadic 
> > arguments, but it also means that we have to introduce the restrictions 
> > that we can only allow packs in the last argument and allow only that 
> > argument to be variadic, as otherwise we have no way of knowing when and 
> > where the packs pass between argument boundaries.
> My point is, I don't think THIS function should be handling the ellipsis, the 
> expression parsing functions we send the parsing of each component off to 
> should do that.
> 
> We unfortunately cannot move the entirety of this parsing section to do that, 
> since 'identifier'/'type', and 'Function' argument types end up needing 
> special handling, but it would be nice if our 'expr' types would all be able 
> to do this, and I suggested earlier that we could start with the 
> `VariadicExprArgument` to make this an easier patch that didn't have to deal 
> with the issues that come with that.
> 
> That said, it would be nice if we could get CLOSE to that.
Sorry. I am still not sure I am following. Do you mean the consumption of pack 
expressions should be moved into the expression parsing so other places (not 
just attributes) would accept it? If so, I am not opposed to the idea though I 
have no idea which expressions these would be nor the full implications of such 
a change. For these we at least have the isolation of having to explicitly 
support the packs in the given attributes.


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

https://reviews.llvm.org/D114439

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114439: [Annotation] Allow parameter pack expansions in annotate attribute

[Steffen Larsen via Phabricator via cfe-commits]

steffenlarsen added inline comments.



Comment at: clang/lib/Parse/ParseDecl.cpp:447
 Actions.CorrectDelayedTyposInExpr(ParseAssignmentExpression()));
+
+if (Tok.is(tok::ellipsis)) {

erichkeane wrote:
> steffenlarsen wrote:
> > erichkeane wrote:
> > > steffenlarsen wrote:
> > > > erichkeane wrote:
> > > > > So I was thinking about this overnight... I wonder if this logic is 
> > > > > inverted here?  Aaron can better answer, but I wonder if we should be 
> > > > > instead detecting when we are on the 'last' parameter and it is one 
> > > > > of these `VariadicExprArgument` things (that accept a pack), then 
> > > > > dropping the parser into a loop of:
> > > > > 
> > > > > while (Tok.is(tok::comma)){
> > > > >   Tok.Consume();
> > > > >   ArgExpr = CorrectTypos(ParseAssignmentExpr(...));
> > > > > }
> > > > > // finally, consume the closing paren.
> > > > > 
> > > > > WDYT?
> > > > The parsing of attribute arguments is already this lenient. It does not 
> > > > actually care how many arguments the attribute says it takes (unless it 
> > > > takes a type and then it will only currently supply _one_ argument). It 
> > > > simply consumes as many expressions it can before reaching the end 
> > > > parenthesis, then leaves the attributes to consume them in 
> > > > `clang/lib/Sema/SemaDeclAttr.cpp`.
> > > > As a result we do not need any additional work here to handle variadic 
> > > > arguments, but it also means that we have to introduce the restrictions 
> > > > that we can only allow packs in the last argument and allow only that 
> > > > argument to be variadic, as otherwise we have no way of knowing when 
> > > > and where the packs pass between argument boundaries.
> > > My point is, I don't think THIS function should be handling the ellipsis, 
> > > the expression parsing functions we send the parsing of each component 
> > > off to should do that.
> > > 
> > > We unfortunately cannot move the entirety of this parsing section to do 
> > > that, since 'identifier'/'type', and 'Function' argument types end up 
> > > needing special handling, but it would be nice if our 'expr' types would 
> > > all be able to do this, and I suggested earlier that we could start with 
> > > the `VariadicExprArgument` to make this an easier patch that didn't have 
> > > to deal with the issues that come with that.
> > > 
> > > That said, it would be nice if we could get CLOSE to that.
> > Sorry. I am still not sure I am following. Do you mean the consumption of 
> > pack expressions should be moved into the expression parsing so other 
> > places (not just attributes) would accept it? If so, I am not opposed to 
> > the idea though I have no idea which expressions these would be nor the 
> > full implications of such a change. For these we at least have the 
> > isolation of having to explicitly support the packs in the given attributes.
> I'm saying the other expression parsers should ALREADY properly handle packs, 
> and we should just use those.
I think I see what you mean. There is the `Parser::ParseExpressionList` which 
may fit the bill, though it seems to also accept initializer lists. But since 
it is an expression the attributes can decide if it is a valid expression for 
them, so maybe that is not a problem?


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

https://reviews.llvm.org/D114439

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114439: [Annotation] Allow parameter pack expansions in annotate attribute

[Steffen Larsen via Phabricator via cfe-commits]

steffenlarsen added a comment.

I agree that reusing existing parser logic for this would be preferable, but as 
@aaron.ballman mentions `Parser::ParseSimpleExpressionList` does not give us 
the parameter pack expansion parsing we need. We could potentially unify it 
with the logic from 
https://github.com/llvm/llvm-project/blob/main/clang/lib/Parse/ParseExpr.cpp#L3059
 but on the other hand there could be potential benefits to using 
`Parser::ParseExpressionList` instead.

The primary difference between the behavior of `Parser::ParseExpressionList` 
and what we are looking for is that it will allow initializer lists as 
arguments. Since most (if not all) attributes check the validity of their 
expression arguments, they would likely fail accordingly if they get an 
initializer list, likely specifying they expected an argument of type X. It may 
be a blessing in disguise to parse these arguments as well as we are more 
permissive w.r.t. attribute arguments. Please let me know if there is anything 
fundamentally wrong with this line of logic. One concern I have is that we 
currently run `Actions.CorrectDelayedTyposInExpr` on the expression right after 
parsing it, then we run pack expansion, whereas `Parser::ParseExpressionList` 
only runs the former action in failure case. I am unsure whether or not that 
might pose a problem.

One problem is that if we only use `Parser::ParseExpressionList` to parse the 
arguments of the last argument (if it is variadic) then we get the odd behavior 
that the initializer lists will only be permitted in variadic arguments, which 
arguably is a little obscure. However, upon further investigation the only case 
that stops us from using this for all expression arguments are when the 
attribute has either `VariadicIdentifierArgument` or 
`VariadicParamOrParamIdxArgument` as the first argument. If we assume that 
these variadic expressions are limited to the last argument as well (as they 
are variadic) leaving them as the only argument of the attributes we are 
parsing here, then we can parse expressions as we did before if we are parsing 
one of these arguments or switch to using e.g. `Parser::ParseExpressionList` 
for all expressions of the attribute (even non-variadic) if not. Then we would 
just have to iterate over the produced expressions to make sure no parameter 
pack expansions occur before the last (variadic) argument of the parsed 
attribute.

//Summary of suggested changes://
Split attribute parsing into 3: First case parsing a type (currently exists). 
Second case keeping the current logic for parsing identifiers and single 
assignment-expressions if the attribute has a single argument of type 
`VariadicIdentifierArgument` or `VariadicParamOrParamIdxArgument`. Third case, 
applicable if none of the former ran, parses all expression parameters in one 
using `Parser::ParseExpressionList` and then checks that parameter packs only 
occur in the last variadic argument (if it allows it). Side effect is it allows 
initializer lists in the third case, but attributes are likely to complain 
about unexpected expression types if these are passed.


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

https://reviews.llvm.org/D114439

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100124: [Clang][NVPTX] Add NVPTX intrinsics and builtins for CUDA PTX redux.sync instructions

[Steffen Larsen via Phabricator via cfe-commits]

steffenlarsen created this revision.
steffenlarsen added reviewers: jdoerfert, jholewinski.
Herald added subscribers: hiraditya, yaxunl.
steffenlarsen requested review of this revision.
Herald added projects: clang, LLVM.
Herald added subscribers: llvm-commits, cfe-commits.

Adds NVPTX builtins and intrinsics for the CUDA PTX `redux.sync` instructions 
for `sm_80` architecture or newer.

PTX ISA description of `redux.sync`: 
https://docs.nvidia.com/cuda/parallel-thread-execution/index.html#parallel-synchronization-and-communication-instructions-redux-sync

Authored-by: Steffen Larsen 


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D100124

Files:
  clang/include/clang/Basic/BuiltinsNVPTX.def
  clang/test/CodeGenCUDA/redux-builtins.cu
  llvm/include/llvm/IR/IntrinsicsNVVM.td
  llvm/lib/Target/NVPTX/NVPTXInstrInfo.td
  llvm/lib/Target/NVPTX/NVPTXIntrinsics.td
  llvm/test/CodeGen/NVPTX/redux-sync.ll

Index: llvm/test/CodeGen/NVPTX/redux-sync.ll
===
--- /dev/null
+++ llvm/test/CodeGen/NVPTX/redux-sync.ll
@@ -0,0 +1,73 @@
+; RUN: llc < %s -march=nvptx64 -mcpu=sm_80 -mattr=+ptx70 | FileCheck %s
+
+declare i32 @llvm.nvvm.redux.sync.add.u32(i32, i32)
+; CHECK-LABEL: .func{{.*}}redux_sync_add_u32
+define i32 @redux_sync_add_u32(i32 %src, i32 %mask) {
+  ; CHECK: redux.sync.add.u32
+  %val = call i32 @llvm.nvvm.redux.sync.add.u32(i32 %src, i32 %mask)
+  ret i32 %val
+}
+
+declare i32 @llvm.nvvm.redux.sync.min.u32(i32, i32)
+; CHECK-LABEL: .func{{.*}}redux_sync_min_u32
+define i32 @redux_sync_min_u32(i32 %src, i32 %mask) {
+  ; CHECK: redux.sync.min.u32
+  %val = call i32 @llvm.nvvm.redux.sync.min.u32(i32 %src, i32 %mask)
+  ret i32 %val
+}
+
+declare i32 @llvm.nvvm.redux.sync.max.u32(i32, i32)
+; CHECK-LABEL: .func{{.*}}redux_sync_max_u32
+define i32 @redux_sync_max_u32(i32 %src, i32 %mask) {
+  ; CHECK: redux.sync.max.u32
+  %val = call i32 @llvm.nvvm.redux.sync.max.u32(i32 %src, i32 %mask)
+  ret i32 %val
+}
+
+declare i32 @llvm.nvvm.redux.sync.add.s32(i32, i32)
+; CHECK-LABEL: .func{{.*}}redux_sync_add_s32
+define i32 @redux_sync_add_s32(i32 %src, i32 %mask) {
+  ; CHECK: redux.sync.add.s32
+  %val = call i32 @llvm.nvvm.redux.sync.add.s32(i32 %src, i32 %mask)
+  ret i32 %val
+}
+
+declare i32 @llvm.nvvm.redux.sync.min.s32(i32, i32)
+; CHECK-LABEL: .func{{.*}}redux_sync_min_s32
+define i32 @redux_sync_min_s32(i32 %src, i32 %mask) {
+  ; CHECK: redux.sync.min.s32
+  %val = call i32 @llvm.nvvm.redux.sync.min.s32(i32 %src, i32 %mask)
+  ret i32 %val
+}
+
+declare i32 @llvm.nvvm.redux.sync.max.s32(i32, i32)
+; CHECK-LABEL: .func{{.*}}redux_sync_max_s32
+define i32 @redux_sync_max_s32(i32 %src, i32 %mask) {
+  ; CHECK: redux.sync.max.s32
+  %val = call i32 @llvm.nvvm.redux.sync.max.s32(i32 %src, i32 %mask)
+  ret i32 %val
+}
+
+declare i32 @llvm.nvvm.redux.sync.and.b32(i32, i32)
+; CHECK-LABEL: .func{{.*}}redux_sync_and_b32
+define i32 @redux_sync_and_b32(i32 %src, i32 %mask) {
+  ; CHECK: redux.sync.and.b32
+  %val = call i32 @llvm.nvvm.redux.sync.and.b32(i32 %src, i32 %mask)
+  ret i32 %val
+}
+
+declare i32 @llvm.nvvm.redux.sync.xor.b32(i32, i32)
+; CHECK-LABEL: .func{{.*}}redux_sync_xor_b32
+define i32 @redux_sync_xor_b32(i32 %src, i32 %mask) {
+  ; CHECK: redux.sync.xor.b32
+  %val = call i32 @llvm.nvvm.redux.sync.xor.b32(i32 %src, i32 %mask)
+  ret i32 %val
+}
+
+declare i32 @llvm.nvvm.redux.sync.or.b32(i32, i32)
+; CHECK-LABEL: .func{{.*}}redux_sync_or_b32
+define i32 @redux_sync_or_b32(i32 %src, i32 %mask) {
+  ; CHECK: redux.sync.or.b32
+  %val = call i32 @llvm.nvvm.redux.sync.or.b32(i32 %src, i32 %mask)
+  ret i32 %val
+}
Index: llvm/lib/Target/NVPTX/NVPTXIntrinsics.td
===
--- llvm/lib/Target/NVPTX/NVPTXIntrinsics.td
+++ llvm/lib/Target/NVPTX/NVPTXIntrinsics.td
@@ -274,6 +274,23 @@
 defm MATCH_ALLP_SYNC_64 : MATCH_ALLP_SYNC;
 
+multiclass REDUX_SYNC {
+  def : NVPTXInst<(outs Int32Regs:$dst), (ins Int32Regs:$src, Int32Regs:$mask),
+  "redux.sync." # BinOp # "." # PTXType # " $dst, $src, $mask;",
+  [(set Int32Regs:$dst, (Intrin Int32Regs:$src, Int32Regs:$mask))]>,
+Requires<[hasPTX70, hasSM80]>;
+}
+
+defm REDUX_SYNC_ADD_U32 : REDUX_SYNC<"add", "u32", int_nvvm_redux_sync_add_u32>;
+defm REDUX_SYNC_MIN_U32 : REDUX_SYNC<"min", "u32", int_nvvm_redux_sync_min_u32>;
+defm REDUX_SYNC_MAX_U32 : REDUX_SYNC<"max", "u32", int_nvvm_redux_sync_max_u32>;
+defm REDUX_SYNC_ADD_S32 : REDUX_SYNC<"add", "s32", int_nvvm_redux_sync_add_s32>;
+defm REDUX_SYNC_MIN_S32 : REDUX_SYNC<"min", "s32", int_nvvm_redux_sync_min_s32>;
+defm REDUX_SYNC_MAX_S32 : REDUX_SYNC<"max", "s32", int_nvvm_redux_sync_max_s32>;
+defm REDUX_SYNC_AND_B32 : REDUX_SYNC<"and", "b32", int_nvvm_redux_sync_and_b32>;
+defm REDUX_SYNC_XOR_B32 : REDUX_SYNC<"xor", "b32", int_nvvm_redux_sync_xor_b32>;
+defm REDUX_SYNC_OR_B32 : REDUX_SYNC<"or", "b32", int_nvvm_redux_sync_or_b32>;
+
 } // isConvergent

[PATCH] D100124: [Clang][NVPTX] Add NVPTX intrinsics and builtins for CUDA PTX redux.sync instructions

[Steffen Larsen via Phabricator via cfe-commits]

steffenlarsen added a comment.

@tra  Thank you for the feedback! I think I see what you're getting at, but I 
am not quite understanding how it would work for these builtins and intrinsics. 
I have added some comments to the corresponding

The comment about `IntrInaccessibleMemOnly` I agree with completely, so I will 
replace `IntrNoMem` with it in the updated version I'm getting ready. Good 
call. :)




Comment at: clang/include/clang/Basic/BuiltinsNVPTX.def:460-468
+TARGET_BUILTIN(__nvvm_redux_sync_add_s32, "SiSii", "", SM_80)
+TARGET_BUILTIN(__nvvm_redux_sync_min_s32, "SiSii", "", SM_80)
+TARGET_BUILTIN(__nvvm_redux_sync_max_s32, "SiSii", "", SM_80)
+TARGET_BUILTIN(__nvvm_redux_sync_add_u32, "UiUii", "", SM_80)
+TARGET_BUILTIN(__nvvm_redux_sync_min_u32, "UiUii", "", SM_80)
+TARGET_BUILTIN(__nvvm_redux_sync_max_u32, "UiUii", "", SM_80)
+TARGET_BUILTIN(__nvvm_redux_sync_and_b32, "iii", "", SM_80)

tra wrote:
> Instead of creating one builtin per integer variant, can we use a more 
> generic builtin `__nvvm_redux_sync_add_i`, similar to how we handle 
> `__nvvm_atom_add_gen_i` ?
> 
What gives me pause is that a for atomic minimum there are both 
`__nvvm_atom_min_gen_i` and `__nvvm_atom_min_gen_ui` to distinguish between 
signed and unsigned. What makes the difference?

That noted, I'll happily rename the builtins to be more in line with the other 
builtins. `__nvvm_redux_sync_*_i` and `__nvvm_redux_sync_*_ui` maybe?



Comment at: llvm/include/llvm/IR/IntrinsicsNVVM.td:4103
+// redux.sync.add.u32 dst, src, membermask;
+def int_nvvm_redux_sync_add_u32 : GCCBuiltin<"__nvvm_redux_sync_add_u32">,
+  Intrinsic<[llvm_i32_ty], [llvm_i32_ty, llvm_i32_ty],

tra wrote:
> This could also be consolidated into an overloaded intrinsic operating on 
> `llvm_anyint_ty`
I am having a look at other uses of this, but I'm having difficulty wrapping my 
head around how to map these overloads to the PTX instructions in 
llvm/lib/Target/NVPTX/NVPTXIntrinsics.td. Though it would be nice, it just 
seems overly complicated to get a signed and an unsigned 32-bit integer version 
of each of these intrinsics.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100124

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100124: [Clang][NVPTX] Add NVPTX intrinsics and builtins for CUDA PTX redux.sync instructions

[Steffen Larsen via Phabricator via cfe-commits]

steffenlarsen updated this revision to Diff 336454.
steffenlarsen added a comment.

Following changes:

- Changed the type in the names of the intrinsics and builtins.
- Changed use of `IntrNoMem` to `IntrInaccessibleMemOnly`.
- Added `PTX70` as a requirement to the builtins.


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

https://reviews.llvm.org/D100124

Files:
  clang/include/clang/Basic/BuiltinsNVPTX.def
  clang/test/CodeGenCUDA/redux-builtins.cu
  llvm/include/llvm/IR/IntrinsicsNVVM.td
  llvm/lib/Target/NVPTX/NVPTXInstrInfo.td
  llvm/lib/Target/NVPTX/NVPTXIntrinsics.td
  llvm/test/CodeGen/NVPTX/redux-sync.ll

Index: llvm/test/CodeGen/NVPTX/redux-sync.ll
===
--- /dev/null
+++ llvm/test/CodeGen/NVPTX/redux-sync.ll
@@ -0,0 +1,73 @@
+; RUN: llc < %s -march=nvptx64 -mcpu=sm_80 -mattr=+ptx70 | FileCheck %s
+
+declare i32 @llvm.nvvm.redux.sync.add.u32(i32, i32)
+; CHECK-LABEL: .func{{.*}}redux_sync_add_u32
+define i32 @redux_sync_add_u32(i32 %src, i32 %mask) {
+  ; CHECK: redux.sync.add.u32
+  %val = call i32 @llvm.nvvm.redux.sync.add.u32(i32 %src, i32 %mask)
+  ret i32 %val
+}
+
+declare i32 @llvm.nvvm.redux.sync.min.u32(i32, i32)
+; CHECK-LABEL: .func{{.*}}redux_sync_min_u32
+define i32 @redux_sync_min_u32(i32 %src, i32 %mask) {
+  ; CHECK: redux.sync.min.u32
+  %val = call i32 @llvm.nvvm.redux.sync.min.u32(i32 %src, i32 %mask)
+  ret i32 %val
+}
+
+declare i32 @llvm.nvvm.redux.sync.max.u32(i32, i32)
+; CHECK-LABEL: .func{{.*}}redux_sync_max_u32
+define i32 @redux_sync_max_u32(i32 %src, i32 %mask) {
+  ; CHECK: redux.sync.max.u32
+  %val = call i32 @llvm.nvvm.redux.sync.max.u32(i32 %src, i32 %mask)
+  ret i32 %val
+}
+
+declare i32 @llvm.nvvm.redux.sync.add.s32(i32, i32)
+; CHECK-LABEL: .func{{.*}}redux_sync_add_s32
+define i32 @redux_sync_add_s32(i32 %src, i32 %mask) {
+  ; CHECK: redux.sync.add.s32
+  %val = call i32 @llvm.nvvm.redux.sync.add.s32(i32 %src, i32 %mask)
+  ret i32 %val
+}
+
+declare i32 @llvm.nvvm.redux.sync.min.s32(i32, i32)
+; CHECK-LABEL: .func{{.*}}redux_sync_min_s32
+define i32 @redux_sync_min_s32(i32 %src, i32 %mask) {
+  ; CHECK: redux.sync.min.s32
+  %val = call i32 @llvm.nvvm.redux.sync.min.s32(i32 %src, i32 %mask)
+  ret i32 %val
+}
+
+declare i32 @llvm.nvvm.redux.sync.max.s32(i32, i32)
+; CHECK-LABEL: .func{{.*}}redux_sync_max_s32
+define i32 @redux_sync_max_s32(i32 %src, i32 %mask) {
+  ; CHECK: redux.sync.max.s32
+  %val = call i32 @llvm.nvvm.redux.sync.max.s32(i32 %src, i32 %mask)
+  ret i32 %val
+}
+
+declare i32 @llvm.nvvm.redux.sync.and.b32(i32, i32)
+; CHECK-LABEL: .func{{.*}}redux_sync_and_b32
+define i32 @redux_sync_and_b32(i32 %src, i32 %mask) {
+  ; CHECK: redux.sync.and.b32
+  %val = call i32 @llvm.nvvm.redux.sync.and.b32(i32 %src, i32 %mask)
+  ret i32 %val
+}
+
+declare i32 @llvm.nvvm.redux.sync.xor.b32(i32, i32)
+; CHECK-LABEL: .func{{.*}}redux_sync_xor_b32
+define i32 @redux_sync_xor_b32(i32 %src, i32 %mask) {
+  ; CHECK: redux.sync.xor.b32
+  %val = call i32 @llvm.nvvm.redux.sync.xor.b32(i32 %src, i32 %mask)
+  ret i32 %val
+}
+
+declare i32 @llvm.nvvm.redux.sync.or.b32(i32, i32)
+; CHECK-LABEL: .func{{.*}}redux_sync_or_b32
+define i32 @redux_sync_or_b32(i32 %src, i32 %mask) {
+  ; CHECK: redux.sync.or.b32
+  %val = call i32 @llvm.nvvm.redux.sync.or.b32(i32 %src, i32 %mask)
+  ret i32 %val
+}
Index: llvm/lib/Target/NVPTX/NVPTXIntrinsics.td
===
--- llvm/lib/Target/NVPTX/NVPTXIntrinsics.td
+++ llvm/lib/Target/NVPTX/NVPTXIntrinsics.td
@@ -274,6 +274,23 @@
 defm MATCH_ALLP_SYNC_64 : MATCH_ALLP_SYNC;
 
+multiclass REDUX_SYNC {
+  def : NVPTXInst<(outs Int32Regs:$dst), (ins Int32Regs:$src, Int32Regs:$mask),
+  "redux.sync." # BinOp # "." # PTXType # " $dst, $src, $mask;",
+  [(set Int32Regs:$dst, (Intrin Int32Regs:$src, Int32Regs:$mask))]>,
+Requires<[hasPTX70, hasSM80]>;
+}
+
+defm REDUX_SYNC_ADD_U32 : REDUX_SYNC<"add", "u32", int_nvvm_redux_sync_add_ui>;
+defm REDUX_SYNC_MIN_U32 : REDUX_SYNC<"min", "u32", int_nvvm_redux_sync_min_ui>;
+defm REDUX_SYNC_MAX_U32 : REDUX_SYNC<"max", "u32", int_nvvm_redux_sync_max_ui>;
+defm REDUX_SYNC_ADD_S32 : REDUX_SYNC<"add", "s32", int_nvvm_redux_sync_add_i>;
+defm REDUX_SYNC_MIN_S32 : REDUX_SYNC<"min", "s32", int_nvvm_redux_sync_min_i>;
+defm REDUX_SYNC_MAX_S32 : REDUX_SYNC<"max", "s32", int_nvvm_redux_sync_max_i>;
+defm REDUX_SYNC_AND_B32 : REDUX_SYNC<"and", "b32", int_nvvm_redux_sync_and_i>;
+defm REDUX_SYNC_XOR_B32 : REDUX_SYNC<"xor", "b32", int_nvvm_redux_sync_xor_i>;
+defm REDUX_SYNC_OR_B32 : REDUX_SYNC<"or", "b32", int_nvvm_redux_sync_or_i>;
+
 } // isConvergent = true
 
 //---
Index: llvm/lib/Target/NVPTX/NVPTXInstrInfo.td
===
--- llvm/lib/Target/NVPTX/NVPTXInstrInfo.td
+++ llvm/lib/Target/NVPTX/NVPTXInstrInfo.td
@@ -144,11 +144,13 @@
 def hasPTX61 : Predicate<"S

[PATCH] D100124: [Clang][NVPTX] Add NVPTX intrinsics and builtins for CUDA PTX redux.sync instructions

[Steffen Larsen via Phabricator via cfe-commits]

steffenlarsen added a comment.

In D100124#2684303 , @JonChesterfield 
wrote:

> Interesting. Reduction across lanes in warp? If so, this is probably a way to 
> handle the last step reduction for openmp reductions

It is! I can imagine that it would be useful for OpenMP reductions, though it 
is limited to few, albeit common, operators on 32-bit integers.


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

https://reviews.llvm.org/D100124

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100124: [Clang][NVPTX] Add NVPTX intrinsics and builtins for CUDA PTX redux.sync instructions

[Steffen Larsen via Phabricator via cfe-commits]

steffenlarsen marked 3 inline comments as done.
steffenlarsen added inline comments.



Comment at: clang/include/clang/Basic/BuiltinsNVPTX.def:460-468
+TARGET_BUILTIN(__nvvm_redux_sync_add_s32, "SiSii", "", SM_80)
+TARGET_BUILTIN(__nvvm_redux_sync_min_s32, "SiSii", "", SM_80)
+TARGET_BUILTIN(__nvvm_redux_sync_max_s32, "SiSii", "", SM_80)
+TARGET_BUILTIN(__nvvm_redux_sync_add_u32, "UiUii", "", SM_80)
+TARGET_BUILTIN(__nvvm_redux_sync_min_u32, "UiUii", "", SM_80)
+TARGET_BUILTIN(__nvvm_redux_sync_max_u32, "UiUii", "", SM_80)
+TARGET_BUILTIN(__nvvm_redux_sync_and_b32, "iii", "", SM_80)

tra wrote:
> steffenlarsen wrote:
> > tra wrote:
> > > Instead of creating one builtin per integer variant, can we use a more 
> > > generic builtin `__nvvm_redux_sync_add_i`, similar to how we handle 
> > > `__nvvm_atom_add_gen_i` ?
> > > 
> > What gives me pause is that a for atomic minimum there are both 
> > `__nvvm_atom_min_gen_i` and `__nvvm_atom_min_gen_ui` to distinguish between 
> > signed and unsigned. What makes the difference?
> > 
> > That noted, I'll happily rename the builtins to be more in line with the 
> > other builtins. `__nvvm_redux_sync_*_i` and `__nvvm_redux_sync_*_ui` maybe?
> > What gives me pause is that a for atomic minimum there are both 
> > __nvvm_atom_min_gen_i and __nvvm_atom_min_gen_ui to distinguish between 
> > signed and unsigned. What makes the difference?
> 
> Good point. We do not need unsigned variant for `add`.  We do need explicit 
> signed and unsigned variants ad LLVM IR integer types do not take signedness 
> into account, and the underlying min/max instructions do.  Maybe, rename 
> min_i/min_ui -> min/umin as LLVM does with atomics? 
> 
> We may skip the `_i` suffix on logical ops as they only apply to integers 
> anyways.
> 
Sorry, I completely missed your responses.

> Maybe, rename min_i/min_ui -> min/umin as LLVM does with atomics?

Sounds good to me. Would there also be umax and uadd?

> We may skip the _i suffix on logical ops as they only apply to integers 
> anyways.

Absolutely. I'll make that happen! 



Comment at: llvm/include/llvm/IR/IntrinsicsNVVM.td:4103
+// redux.sync.add.u32 dst, src, membermask;
+def int_nvvm_redux_sync_add_u32 : GCCBuiltin<"__nvvm_redux_sync_add_u32">,
+  Intrinsic<[llvm_i32_ty], [llvm_i32_ty, llvm_i32_ty],

tra wrote:
> steffenlarsen wrote:
> > tra wrote:
> > > This could also be consolidated into an overloaded intrinsic operating on 
> > > `llvm_anyint_ty`
> > I am having a look at other uses of this, but I'm having difficulty 
> > wrapping my head around how to map these overloads to the PTX instructions 
> > in llvm/lib/Target/NVPTX/NVPTXIntrinsics.td. Though it would be nice, it 
> > just seems overly complicated to get a signed and an unsigned 32-bit 
> > integer version of each of these intrinsics.
> Considering that `redux` only supports 32-bit integers, we do not need to get 
> into it.
> `llvm_i32_ty` will do for now. We'll cross the bridge if/when we get to 
> support multiple integer types.
> 
> 
Perfect, thank you!


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

https://reviews.llvm.org/D100124

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100124: [Clang][NVPTX] Add NVPTX intrinsics and builtins for CUDA PTX redux.sync instructions

[Steffen Larsen via Phabricator via cfe-commits]

steffenlarsen added inline comments.



Comment at: clang/include/clang/Basic/BuiltinsNVPTX.def:460-468
+TARGET_BUILTIN(__nvvm_redux_sync_add_s32, "SiSii", "", SM_80)
+TARGET_BUILTIN(__nvvm_redux_sync_min_s32, "SiSii", "", SM_80)
+TARGET_BUILTIN(__nvvm_redux_sync_max_s32, "SiSii", "", SM_80)
+TARGET_BUILTIN(__nvvm_redux_sync_add_u32, "UiUii", "", SM_80)
+TARGET_BUILTIN(__nvvm_redux_sync_min_u32, "UiUii", "", SM_80)
+TARGET_BUILTIN(__nvvm_redux_sync_max_u32, "UiUii", "", SM_80)
+TARGET_BUILTIN(__nvvm_redux_sync_and_b32, "iii", "", SM_80)

tra wrote:
> steffenlarsen wrote:
> > tra wrote:
> > > steffenlarsen wrote:
> > > > tra wrote:
> > > > > Instead of creating one builtin per integer variant, can we use a 
> > > > > more generic builtin `__nvvm_redux_sync_add_i`, similar to how we 
> > > > > handle `__nvvm_atom_add_gen_i` ?
> > > > > 
> > > > What gives me pause is that a for atomic minimum there are both 
> > > > `__nvvm_atom_min_gen_i` and `__nvvm_atom_min_gen_ui` to distinguish 
> > > > between signed and unsigned. What makes the difference?
> > > > 
> > > > That noted, I'll happily rename the builtins to be more in line with 
> > > > the other builtins. `__nvvm_redux_sync_*_i` and 
> > > > `__nvvm_redux_sync_*_ui` maybe?
> > > > What gives me pause is that a for atomic minimum there are both 
> > > > __nvvm_atom_min_gen_i and __nvvm_atom_min_gen_ui to distinguish between 
> > > > signed and unsigned. What makes the difference?
> > > 
> > > Good point. We do not need unsigned variant for `add`.  We do need 
> > > explicit signed and unsigned variants ad LLVM IR integer types do not 
> > > take signedness into account, and the underlying min/max instructions do. 
> > >  Maybe, rename min_i/min_ui -> min/umin as LLVM does with atomics? 
> > > 
> > > We may skip the `_i` suffix on logical ops as they only apply to integers 
> > > anyways.
> > > 
> > Sorry, I completely missed your responses.
> > 
> > > Maybe, rename min_i/min_ui -> min/umin as LLVM does with atomics?
> > 
> > Sounds good to me. Would there also be umax and uadd?
> > 
> > > We may skip the _i suffix on logical ops as they only apply to integers 
> > > anyways.
> > 
> > Absolutely. I'll make that happen! 
> > Would there also be umax and uadd?
> 
> You will need `umax`, but there's no need for `uadd` as 2-complement addition 
> is the same for signed/unsigned.
> 
> E.g `umax(0x, 1) -> 0x`, `max(-1,1) -> 1`, give different 
> answers, but `uadd(0x, 1) -> 0` and `add(-1,1) -> 0`.
Ah, of course. Though I do wonder as to the motivation of having signed and 
unsigned add variants in PTX. I'll drop the unsigned variant.


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

https://reviews.llvm.org/D100124

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100124: [Clang][NVPTX] Add NVPTX intrinsics and builtins for CUDA PTX redux.sync instructions

[Steffen Larsen via Phabricator via cfe-commits]

steffenlarsen updated this revision to Diff 339169.
steffenlarsen added a comment.

Changes:

- Removed integer type from builtin and intrinsic names.
- Signedness in builtin and intrinsic names moved to operator name, i.e. umin 
and umax.
- Removed redundant addition variant.


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

https://reviews.llvm.org/D100124

Files:
  clang/include/clang/Basic/BuiltinsNVPTX.def
  clang/test/CodeGenCUDA/redux-builtins.cu
  llvm/include/llvm/IR/IntrinsicsNVVM.td
  llvm/lib/Target/NVPTX/NVPTXInstrInfo.td
  llvm/lib/Target/NVPTX/NVPTXIntrinsics.td
  llvm/test/CodeGen/NVPTX/redux-sync.ll

Index: llvm/test/CodeGen/NVPTX/redux-sync.ll
===
--- /dev/null
+++ llvm/test/CodeGen/NVPTX/redux-sync.ll
@@ -0,0 +1,65 @@
+; RUN: llc < %s -march=nvptx64 -mcpu=sm_80 -mattr=+ptx70 | FileCheck %s
+
+declare i32 @llvm.nvvm.redux.sync.umin(i32, i32)
+; CHECK-LABEL: .func{{.*}}redux_sync_min_u32
+define i32 @redux_sync_min_u32(i32 %src, i32 %mask) {
+  ; CHECK: redux.sync.min.u32
+  %val = call i32 @llvm.nvvm.redux.sync.umin(i32 %src, i32 %mask)
+  ret i32 %val
+}
+
+declare i32 @llvm.nvvm.redux.sync.umax(i32, i32)
+; CHECK-LABEL: .func{{.*}}redux_sync_max_u32
+define i32 @redux_sync_max_u32(i32 %src, i32 %mask) {
+  ; CHECK: redux.sync.max.u32
+  %val = call i32 @llvm.nvvm.redux.sync.umax(i32 %src, i32 %mask)
+  ret i32 %val
+}
+
+declare i32 @llvm.nvvm.redux.sync.add(i32, i32)
+; CHECK-LABEL: .func{{.*}}redux_sync_add_s32
+define i32 @redux_sync_add_s32(i32 %src, i32 %mask) {
+  ; CHECK: redux.sync.add.s32
+  %val = call i32 @llvm.nvvm.redux.sync.add(i32 %src, i32 %mask)
+  ret i32 %val
+}
+
+declare i32 @llvm.nvvm.redux.sync.min(i32, i32)
+; CHECK-LABEL: .func{{.*}}redux_sync_min_s32
+define i32 @redux_sync_min_s32(i32 %src, i32 %mask) {
+  ; CHECK: redux.sync.min.s32
+  %val = call i32 @llvm.nvvm.redux.sync.min(i32 %src, i32 %mask)
+  ret i32 %val
+}
+
+declare i32 @llvm.nvvm.redux.sync.max(i32, i32)
+; CHECK-LABEL: .func{{.*}}redux_sync_max_s32
+define i32 @redux_sync_max_s32(i32 %src, i32 %mask) {
+  ; CHECK: redux.sync.max.s32
+  %val = call i32 @llvm.nvvm.redux.sync.max(i32 %src, i32 %mask)
+  ret i32 %val
+}
+
+declare i32 @llvm.nvvm.redux.sync.and(i32, i32)
+; CHECK-LABEL: .func{{.*}}redux_sync_and_b32
+define i32 @redux_sync_and_b32(i32 %src, i32 %mask) {
+  ; CHECK: redux.sync.and.b32
+  %val = call i32 @llvm.nvvm.redux.sync.and(i32 %src, i32 %mask)
+  ret i32 %val
+}
+
+declare i32 @llvm.nvvm.redux.sync.xor(i32, i32)
+; CHECK-LABEL: .func{{.*}}redux_sync_xor_b32
+define i32 @redux_sync_xor_b32(i32 %src, i32 %mask) {
+  ; CHECK: redux.sync.xor.b32
+  %val = call i32 @llvm.nvvm.redux.sync.xor(i32 %src, i32 %mask)
+  ret i32 %val
+}
+
+declare i32 @llvm.nvvm.redux.sync.or(i32, i32)
+; CHECK-LABEL: .func{{.*}}redux_sync_or_b32
+define i32 @redux_sync_or_b32(i32 %src, i32 %mask) {
+  ; CHECK: redux.sync.or.b32
+  %val = call i32 @llvm.nvvm.redux.sync.or(i32 %src, i32 %mask)
+  ret i32 %val
+}
Index: llvm/lib/Target/NVPTX/NVPTXIntrinsics.td
===
--- llvm/lib/Target/NVPTX/NVPTXIntrinsics.td
+++ llvm/lib/Target/NVPTX/NVPTXIntrinsics.td
@@ -274,6 +274,22 @@
 defm MATCH_ALLP_SYNC_64 : MATCH_ALLP_SYNC;
 
+multiclass REDUX_SYNC {
+  def : NVPTXInst<(outs Int32Regs:$dst), (ins Int32Regs:$src, Int32Regs:$mask),
+  "redux.sync." # BinOp # "." # PTXType # " $dst, $src, $mask;",
+  [(set Int32Regs:$dst, (Intrin Int32Regs:$src, Int32Regs:$mask))]>,
+Requires<[hasPTX70, hasSM80]>;
+}
+
+defm REDUX_SYNC_UMIN : REDUX_SYNC<"min", "u32", int_nvvm_redux_sync_umin>;
+defm REDUX_SYNC_UMAX : REDUX_SYNC<"max", "u32", int_nvvm_redux_sync_umax>;
+defm REDUX_SYNC_ADD : REDUX_SYNC<"add", "s32", int_nvvm_redux_sync_add>;
+defm REDUX_SYNC_MIN : REDUX_SYNC<"min", "s32", int_nvvm_redux_sync_min>;
+defm REDUX_SYNC_MAX : REDUX_SYNC<"max", "s32", int_nvvm_redux_sync_max>;
+defm REDUX_SYNC_AND : REDUX_SYNC<"and", "b32", int_nvvm_redux_sync_and>;
+defm REDUX_SYNC_XOR : REDUX_SYNC<"xor", "b32", int_nvvm_redux_sync_xor>;
+defm REDUX_SYNC_OR : REDUX_SYNC<"or", "b32", int_nvvm_redux_sync_or>;
+
 } // isConvergent = true
 
 //---
Index: llvm/lib/Target/NVPTX/NVPTXInstrInfo.td
===
--- llvm/lib/Target/NVPTX/NVPTXInstrInfo.td
+++ llvm/lib/Target/NVPTX/NVPTXInstrInfo.td
@@ -144,11 +144,13 @@
 def hasPTX61 : Predicate<"Subtarget->getPTXVersion() >= 61">;
 def hasPTX63 : Predicate<"Subtarget->getPTXVersion() >= 63">;
 def hasPTX64 : Predicate<"Subtarget->getPTXVersion() >= 64">;
+def hasPTX70 : Predicate<"Subtarget->getPTXVersion() >= 70">;
 
 def hasSM30 : Predicate<"Subtarget->getSmVersion() >= 30">;
 def hasSM70 : Predicate<"Subtarget->getSmVersion() >= 70">;
 def hasSM72 : Predicate<"Subtarget->getSmVersion() >= 72">;
 def hasSM75 : Predicate<"Subtarget->getSmVersion() >

[PATCH] D100124: [Clang][NVPTX] Add NVPTX intrinsics and builtins for CUDA PTX redux.sync instructions

[Steffen Larsen via Phabricator via cfe-commits]

steffenlarsen added a comment.

> Do you know if any existing code already uses the __nvvm_* builtins for 
> cp.async? In other words, does nvcc provide them already or is it something 
> we're free to name as we wish? I do not see any relevant intrinsics mentioned 
> in NVVM IR spec: https://docs.nvidia.com/cuda/nvvm-ir-spec/index.html and I 
> don't think NVCC's builtins are publicly documented anywhere.

I don't know of any yet. We will be using these in the relatively near future, 
but we can still change them no problem. However, the intrinsic and builtin 
naming for NVVM and NVPTX seems a bit inconsistent so it may be a long 
discussion (or maybe not.)




Comment at: clang/include/clang/Basic/BuiltinsNVPTX.def:460-468
+TARGET_BUILTIN(__nvvm_redux_sync_add_s32, "SiSii", "", SM_80)
+TARGET_BUILTIN(__nvvm_redux_sync_min_s32, "SiSii", "", SM_80)
+TARGET_BUILTIN(__nvvm_redux_sync_max_s32, "SiSii", "", SM_80)
+TARGET_BUILTIN(__nvvm_redux_sync_add_u32, "UiUii", "", SM_80)
+TARGET_BUILTIN(__nvvm_redux_sync_min_u32, "UiUii", "", SM_80)
+TARGET_BUILTIN(__nvvm_redux_sync_max_u32, "UiUii", "", SM_80)
+TARGET_BUILTIN(__nvvm_redux_sync_and_b32, "iii", "", SM_80)

tra wrote:
> steffenlarsen wrote:
> > tra wrote:
> > > steffenlarsen wrote:
> > > > tra wrote:
> > > > > steffenlarsen wrote:
> > > > > > tra wrote:
> > > > > > > Instead of creating one builtin per integer variant, can we use a 
> > > > > > > more generic builtin `__nvvm_redux_sync_add_i`, similar to how we 
> > > > > > > handle `__nvvm_atom_add_gen_i` ?
> > > > > > > 
> > > > > > What gives me pause is that a for atomic minimum there are both 
> > > > > > `__nvvm_atom_min_gen_i` and `__nvvm_atom_min_gen_ui` to distinguish 
> > > > > > between signed and unsigned. What makes the difference?
> > > > > > 
> > > > > > That noted, I'll happily rename the builtins to be more in line 
> > > > > > with the other builtins. `__nvvm_redux_sync_*_i` and 
> > > > > > `__nvvm_redux_sync_*_ui` maybe?
> > > > > > What gives me pause is that a for atomic minimum there are both 
> > > > > > __nvvm_atom_min_gen_i and __nvvm_atom_min_gen_ui to distinguish 
> > > > > > between signed and unsigned. What makes the difference?
> > > > > 
> > > > > Good point. We do not need unsigned variant for `add`.  We do need 
> > > > > explicit signed and unsigned variants ad LLVM IR integer types do not 
> > > > > take signedness into account, and the underlying min/max instructions 
> > > > > do.  Maybe, rename min_i/min_ui -> min/umin as LLVM does with 
> > > > > atomics? 
> > > > > 
> > > > > We may skip the `_i` suffix on logical ops as they only apply to 
> > > > > integers anyways.
> > > > > 
> > > > Sorry, I completely missed your responses.
> > > > 
> > > > > Maybe, rename min_i/min_ui -> min/umin as LLVM does with atomics?
> > > > 
> > > > Sounds good to me. Would there also be umax and uadd?
> > > > 
> > > > > We may skip the _i suffix on logical ops as they only apply to 
> > > > > integers anyways.
> > > > 
> > > > Absolutely. I'll make that happen! 
> > > > Would there also be umax and uadd?
> > > 
> > > You will need `umax`, but there's no need for `uadd` as 2-complement 
> > > addition is the same for signed/unsigned.
> > > 
> > > E.g `umax(0x, 1) -> 0x`, `max(-1,1) -> 1`, give different 
> > > answers, but `uadd(0x, 1) -> 0` and `add(-1,1) -> 0`.
> > Ah, of course. Though I do wonder as to the motivation of having signed and 
> > unsigned add variants in PTX. I'll drop the unsigned variant.
> It's for uniformity sake, I guess. All arithmetic ops in PTX operate on 
> sXX/uXX arguments, though not all of them have to.
> 
I bet you're right. Thanks for the help. 😄 


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

https://reviews.llvm.org/D100124

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits