address review comments

ghehg · ghehg · commit 5ea324ad9b55 · 2024-09-05T06:24:52.000-07:00
diff --git a/clang/include/clang/CIR/Dialect/IR/CIRAttrs.td b/clang/include/clang/CIR/Dialect/IR/CIRAttrs.td
@@ -1104,19 +1104,20 @@ def BitfieldInfoAttr : CIR_Attr<"BitfieldInfo", "bitfield_info"> {
 //===----------------------------------------------------------------------===//
 // AnnotationAttr
 //===----------------------------------------------------------------------===//
+
 def AnnotationAttr : CIR_Attr<"Annotation", "annotation"> {
   let summary = "Annotation attribute for global variables and functions";
   let description = [{
-    Represent C/C++ attribute of annotate in CIR. example c code is:
+    Represent C/C++ attribute of annotate in CIR. Example c code is:
     ```
-     in test.c line 10
      int *a __attribute__((annotate("testptr", "21", 12 ))); 
     ```
-     this example code, the AnnotationAttr records annotation name "testptr", 
-     and arguments "21" and 12 as ArrayAttr args for global variable a. 
+    In this example code, the `AnnotationAttr` records annotation name "testptr", 
+    and arguments "21" and 12 as `ArrayAttr` type parameter `args`
+    for global variable `a`. 
   }];
 
-  // args is empty when there is no arg.
+  // The parameter args is empty when there is no arg.
   let parameters = (ins "StringAttr":$name,
                         "ArrayAttr":$args);
 
@@ -1128,27 +1129,26 @@ def AnnotationAttr : CIR_Attr<"Annotation", "annotation"> {
 }
 
 //===----------------------------------------------------------------------===//
-// AnnotationValueAttr
+// GlobalAnnotationValueAttr
 //===----------------------------------------------------------------------===//
-def AnnotationValueAttr : CIR_Attr<"AnnotationValue", "annotation value"> {
+
+def GlobalAnnotationValueAttr : CIR_Attr<"GlobalAnnotationValue", "annotation value"> {
   let summary = "An annotation value, consist of name of a global var or func"
                 "and one of its annotations";
   let description = [{
     This is element type of annotation value array, which holds the annotation 
     values for all global variables and functions in a module.
-    This array is used to create the initial value of a global annotation 
+    This array is used to create the initial value of a global annotation
     metadata variable in LLVM IR.
   }];
 
   // Here the name is the name of a global var or func.
-  let parameters = (ins "StringAttr":$name,
-                        "AnnotationAttr":$value);
+  let parameters = (ins "ArrayAttr":$value);
 
+  let assemblyFormat = " $value ";
 
-  let assemblyFormat = "`<` struct($name, $value) `>`"; 
-  
   // Enable verifier.
-  let genVerifyDecl = 1;                       
+  let genVerifyDecl = 1;                      
 }
 
 include "clang/CIR/Dialect/IR/CIROpenCLAttrs.td"
diff --git a/clang/include/clang/CIR/Dialect/IR/CIROps.td b/clang/include/clang/CIR/Dialect/IR/CIROps.td
@@ -2171,7 +2171,7 @@ def GlobalLinkageKind : I32EnumAttr<
     "Linkage type/kind",
     [Global_ExternalLinkage, Global_AvailableExternallyLinkage,
      Global_LinkOnceAnyLinkage, Global_LinkOnceODRLinkage,
-     Global_WeakAnyLinkage, Global_WeakODRLinkage, 
+     Global_WeakAnyLinkage, Global_WeakODRLinkage,
      Global_InternalLinkage, Global_PrivateLinkage,
      Global_ExternalWeakLinkage, Global_CommonLinkage
      ]> {
diff --git a/clang/lib/CIR/CodeGen/CIRGenModule.cpp b/clang/lib/CIR/CodeGen/CIRGenModule.cpp
@@ -3190,38 +3190,29 @@ LangAS CIRGenModule::getGlobalVarAddressSpace(const VarDecl *D) {
   return getTargetCIRGenInfo().getGlobalVarAddressSpace(*this, D);
 }
 
-mlir::StringAttr CIRGenModule::buildAnnotationString(StringRef Str) {
-  auto &Astr = AnnotationStrings[Str];
-  if (Astr)
-    return Astr;
-  Astr = builder.getStringAttr(Str);
-  return Astr;
-}
-
-mlir::ArrayAttr CIRGenModule::buildAnnotationArgs(const AnnotateAttr *Attr) {
-  ArrayRef<Expr *> Exprs = {Attr->args_begin(), Attr->args_size()};
-  if (Exprs.empty()) {
+mlir::ArrayAttr CIRGenModule::buildAnnotationArgs(const AnnotateAttr *attr) {
+  ArrayRef<Expr *> exprs = {attr->args_begin(), attr->args_size()};
+  if (exprs.empty()) {
     return mlir::ArrayAttr::get(builder.getContext(), {});
   }
-  llvm::FoldingSetNodeID ID;
-  for (Expr *E : Exprs) {
-    ID.Add(cast<clang::ConstantExpr>(E)->getAPValueResult());
+  llvm::FoldingSetNodeID id;
+  for (Expr *e : exprs) {
+    id.Add(cast<clang::ConstantExpr>(e)->getAPValueResult());
   }
-  auto &Lookup = AnnotationArgs[ID.ComputeHash()];
-  if (Lookup)
-    return Lookup;
+  mlir::ArrayAttr &lookup = AnnotationArgs[id.ComputeHash()];
+  if (lookup)
+    return lookup;
 
-  llvm::SmallVector<mlir::Attribute, 4> Args;
-  Args.reserve(Exprs.size());
-  for (Expr *E : Exprs) {
+  llvm::SmallVector<mlir::Attribute, 4> args;
+  args.reserve(exprs.size());
+  for (Expr *e : exprs) {
     if (const auto StrE =
-            ::clang::dyn_cast<clang::StringLiteral>(E->IgnoreParenCasts())) {
+            ::clang::dyn_cast<clang::StringLiteral>(e->IgnoreParenCasts())) {
       // Add trailing null character as StringLiteral->getString() does not
-      Args.push_back(
-          buildAnnotationString(std::string(StrE->getString()) + '\0'));
+      args.push_back(builder.getStringAttr(StrE->getString()));
     } else if (const auto IntE = ::clang::dyn_cast<clang::IntegerLiteral>(
-                   E->IgnoreParenCasts())) {
-      Args.push_back(mlir::IntegerAttr::get(
+                   e->IgnoreParenCasts())) {
+      args.push_back(mlir::IntegerAttr::get(
           mlir::IntegerType::get(builder.getContext(),
                                  IntE->getValue().getBitWidth()),
           IntE->getValue()));
@@ -3230,32 +3221,30 @@ mlir::ArrayAttr CIRGenModule::buildAnnotationArgs(const AnnotateAttr *Attr) {
     }
   }
   // Create const struct attr for these arguments
-  mlir::ArrayAttr ArgsArrayAttr = builder.getArrayAttr(Args);
-  Lookup = ArgsArrayAttr;
-  return Lookup;
+  mlir::ArrayAttr argsArrayAttr = builder.getArrayAttr(args);
+  lookup = argsArrayAttr;
+  return lookup;
 }
 
 mlir::cir::AnnotationAttr
-CIRGenModule::buildAnnotateAttr(mlir::Operation *GV, const AnnotateAttr *AA,
-                                SourceLocation L) {
-
-  auto AnnoGV = buildAnnotationString(std::string(AA->getAnnotation()) + '\0');
-  auto Args = buildAnnotationArgs(AA);
-  return mlir::cir::AnnotationAttr::get(builder.getContext(), AnnoGV, Args);
+CIRGenModule::buildAnnotateAttr(const AnnotateAttr *aa) {
+  auto annoGV = builder.getStringAttr(aa->getAnnotation());
+  mlir::ArrayAttr args = buildAnnotationArgs(aa);
+  return mlir::cir::AnnotationAttr::get(builder.getContext(), annoGV, args);
 }
 
-void CIRGenModule::addGlobalAnnotations(const ValueDecl *D,
-                                        mlir::Operation *GV) {
-  assert(D->hasAttr<AnnotateAttr>() && "no annotate attribute");
-  assert((isa<GlobalOp>(GV) || isa<FuncOp>(GV)) &&
+void CIRGenModule::addGlobalAnnotations(const ValueDecl *d,
+                                        mlir::Operation *gv) {
+  assert(d->hasAttr<AnnotateAttr>() && "no annotate attribute");
+  assert((isa<GlobalOp>(gv) || isa<FuncOp>(gv)) &&
          "annotation only on globals");
-  llvm::SmallVector<mlir::Attribute, 4> Annotations;
-  for (const auto *I : D->specific_attrs<AnnotateAttr>())
-    Annotations.push_back(buildAnnotateAttr(GV, I, D->getLocation()));
-  if (auto global = dyn_cast<mlir::cir::GlobalOp>(GV))
-    global.setAnnotationsAttr(builder.getArrayAttr(Annotations));
-  else if (auto func = dyn_cast<mlir::cir::FuncOp>(GV))
-    func.setAnnotationsAttr(builder.getArrayAttr(Annotations));
+  llvm::SmallVector<mlir::Attribute, 4> annotations;
+  for (const auto *i : d->specific_attrs<AnnotateAttr>())
+    annotations.push_back(buildAnnotateAttr(i));
+  if (auto global = dyn_cast<mlir::cir::GlobalOp>(gv))
+    global.setAnnotationsAttr(builder.getArrayAttr(annotations));
+  else if (auto func = dyn_cast<mlir::cir::FuncOp>(gv))
+    func.setAnnotationsAttr(builder.getArrayAttr(annotations));
 }
 
 void CIRGenModule::buildGlobalAnnotations() {
diff --git a/clang/lib/CIR/CodeGen/CIRGenModule.h b/clang/lib/CIR/CodeGen/CIRGenModule.h
@@ -783,24 +783,18 @@ class CIRGenModule : public CIRGenTypeCache {
   /// functions, because global variables need to deffred emission.
   void buildGlobalAnnotations();
 
-  /// Emit an annotation string as the returned StringAttr is needed to
-  /// assemble AnnotationAttr for a GlobalOp or FuncOp.
-  mlir::StringAttr buildAnnotationString(StringRef Str);
-
   /// Emit additional args of the annotation.
-  mlir::ArrayAttr buildAnnotationArgs(const AnnotateAttr *Attr);
+  mlir::ArrayAttr buildAnnotationArgs(const AnnotateAttr *attr);
 
   /// Create cir::AnnotationAttr which contains the annotation
   /// information for a given GlobalValue. Notice that a GlobalValue could
   /// have multiple annotations, and this function creates attribute for
   /// one of them.
-  mlir::cir::AnnotationAttr buildAnnotateAttr(mlir::Operation *GV,
-                                              const AnnotateAttr *AA,
-                                              SourceLocation L);
+  mlir::cir::AnnotationAttr buildAnnotateAttr(const AnnotateAttr *aa);
 
   /// Add global annotations that are set on D, for the global GV. Those
   /// annotations are emitted during lowering to the LLVM code.
-  void addGlobalAnnotations(const ValueDecl *D, mlir::Operation *GV);
+  void addGlobalAnnotations(const ValueDecl *d, mlir::Operation *gv);
 };
 } // namespace cir
 
diff --git a/clang/lib/CIR/Dialect/IR/CIRAttrs.cpp b/clang/lib/CIR/Dialect/IR/CIRAttrs.cpp
@@ -545,13 +545,24 @@ void MethodAttr::print(AsmPrinter &printer) const {
 }
 
 //===----------------------------------------------------------------------===//
-// AnnotationValueAttr definitions
+// GlobalAnnotationValueAttr definitions
 //===----------------------------------------------------------------------===//
-LogicalResult AnnotationValueAttr::verify(
-    function_ref<::mlir::InFlightDiagnostic()> emitError, mlir::StringAttr name,
-    mlir::cir::AnnotationAttr value) {
-  if (name.empty()) {
-    emitError() << "annotation has to be associated with a global variable";
+LogicalResult GlobalAnnotationValueAttr::verify(
+    function_ref<::mlir::InFlightDiagnostic()> emitError,
+    mlir::ArrayAttr value) {
+  if (value.size() < 2) {
+    emitError()
+        << "GlobalAnnotationValueAttr should at least have two elements";
+    return failure();
+  } else if (!::mlir::isa<mlir::StringAttr>(value[0])) {
+    emitError()
+        << "The first element of GlobalAnnotationValueAttr must be string";
+    return failure();
+  }
+  auto annoPart = ::mlir::cast<mlir::cir::AnnotationAttr>(value[1]);
+  if (!annoPart) {
+    emitError() << "The second element of GlobalAnnotationValueAttr must be "
+                   "AnnotationValueAttr";
     return failure();
   }
   return success();
diff --git a/clang/lib/CIR/Dialect/Transforms/LoweringPrepare.cpp b/clang/lib/CIR/Dialect/Transforms/LoweringPrepare.cpp
@@ -156,7 +156,7 @@ struct LoweringPreparePass : public LoweringPrepareBase<LoweringPreparePass> {
   /// as we need view to annotated global values to create global annotation var
   /// later in LLVM lowering.
   //  One global value could have multiple AnnotateAttr
-  std::vector<std::pair<mlir::Attribute, mlir::Operation *>> globalAnnotations;
+  SmallVector<std::pair<mlir::Attribute, mlir::Operation *>> globalAnnotations;
 };
 } // namespace
 
@@ -1080,15 +1080,19 @@ void LoweringPreparePass::lowerIterEndOp(IterEndOp op) {
 void LoweringPreparePass::buildGlobalAnnotationValues() {
   if (globalAnnotations.empty())
     return;
-  std::vector<mlir::Attribute> annotationValueVec;
+  SmallVector<mlir::Attribute> annotationValueVec;
+  annotationValueVec.reserve(globalAnnotations.size());
 
   for (auto &annotEntry : globalAnnotations) {
     auto annot = dyn_cast<mlir::cir::AnnotationAttr>(annotEntry.first);
-    auto op = annotEntry.second;
+    mlir::Operation *op = annotEntry.second;
     auto globalValue = dyn_cast<mlir::SymbolOpInterface>(op);
-    auto globalValueName = globalValue.getNameAttr();
-    auto valueEntry = mlir::cir::AnnotationValueAttr::get(
-        theModule.getContext(), globalValueName, annot);
+    mlir::StringAttr globalValueName = globalValue.getNameAttr();
+    SmallVector<mlir::Attribute, 2> entryArray = {globalValueName, annot};
+    mlir::cir::GlobalAnnotationValueAttr valueEntry =
+        mlir::cir::GlobalAnnotationValueAttr::get(
+            theModule.getContext(),
+            mlir::ArrayAttr::get(theModule.getContext(), entryArray));
     annotationValueVec.push_back(valueEntry);
   }
   mlir::ArrayAttr annotationValueArray =
diff --git a/clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp b/clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp
diff --git a/clang/test/CIR/CodeGen/attribute-annotate-multiple.cpp b/clang/test/CIR/CodeGen/attribute-annotate-multiple.cpp
