Handle review comments.

Mostly formatting nits and adding comments.

Author: abidh

mlir/include/mlir-c/Dialect/LLVM.h

@@ -357,8 +357,8 @@ MLIR_CAPI_EXPORTED MlirAttribute mlirLLVMDIModuleAttrGet(
 /// Creates a LLVM DIImportedEntityAttr attribute.
 MLIR_CAPI_EXPORTED MlirAttribute mlirLLVMDIImportedEntityAttrGet(
     MlirContext ctx, unsigned int tag, MlirAttribute entity, MlirAttribute file,
-    unsigned int line, MlirAttribute name, intptr_t nNodes,
-    MlirAttribute const *elements);
+    unsigned int line, MlirAttribute name, intptr_t nRetainedNodes,
+    MlirAttribute const *retainedNodes);
 
 /// Gets the scope of this DIModuleAttr.
 MLIR_CAPI_EXPORTED MlirAttribute

mlir/include/mlir/Dialect/LLVMIR/LLVMAttrDefs.td

@@ -627,6 +627,11 @@ def LLVM_DINamespaceAttr : LLVM_Attr<"DINamespace", "di_namespace",
 
 def LLVM_DIImportedEntityAttr : LLVM_Attr<"DIImportedEntity", "di_imported_entity",
                                            /*traits=*/[], "DINodeAttr"> {
+  /// TODO: DIImportedEntity has a 'scope' field which represents the scope where
+  /// this entity is imported. Currently, we are not adding a 'scope' field in
+  /// DIImportedEntityAttr to avoid cyclic dependency. As DIImportedEntityAttr
+  /// entries will be contained inside a scope entity (e.g. DISubprogramAttr),
+  /// the scope can easily be inferred.
   let parameters = (ins
     LLVM_DITagParameter:$tag,
     "DINodeAttr":$entity,

mlir/lib/CAPI/Dialect/LLVM.cpp

@@ -353,13 +353,14 @@ MlirAttribute mlirLLVMDIModuleAttrGetScope(MlirAttribute diModule) {
 
 MlirAttribute mlirLLVMDIImportedEntityAttrGet(
     MlirContext ctx, unsigned int tag, MlirAttribute entity, MlirAttribute file,
-    unsigned int line, MlirAttribute name, intptr_t nElements,
-    MlirAttribute const *elements) {
-  SmallVector<Attribute> elementsStorage;
-  elementsStorage.reserve(nElements);
+    unsigned int line, MlirAttribute name, intptr_t nRetainedNodes,
+    MlirAttribute const *retainedNodes) {
+  SmallVector<Attribute> nodesStorage;
+  nodesStorage.reserve(nRetainedNodes);
   return wrap(DIImportedEntityAttr::get(
       unwrap(ctx), tag, cast<DINodeAttr>(unwrap(entity)),
       cast<DIFileAttr>(unwrap(file)), line, cast<StringAttr>(unwrap(name)),
-      llvm::map_to_vector(unwrapList(nElements, elements, elementsStorage),
-                          [](Attribute a) { return cast<DINodeAttr>(a); })));
+      llvm::map_to_vector(
+          unwrapList(nRetainedNodes, retainedNodes, nodesStorage),
+          [](Attribute a) { return cast<DINodeAttr>(a); })));
 }

mlir/lib/Dialect/LLVMIR/Transforms/DIScopeForLLVMFuncOp.cpp

@@ -79,7 +79,8 @@ static void addScopeToFunction(LLVM::LLVMFuncOp llvmFunc,
       context, id, compileUnitAttr, fileAttr, funcNameAttr, funcNameAttr,
       fileAttr,
       /*line=*/line,
-      /*scopeline=*/col, subprogramFlags, subroutineTypeAttr, {});
+      /*scopeline=*/col, subprogramFlags, subroutineTypeAttr,
+      /*retainedNodes=*/{});
   llvmFunc->setLoc(FusedLoc::get(context, {loc}, subprogramAttr));
 }
 

mlir/lib/Target/LLVMIR/DebugImporter.cpp

@@ -211,7 +211,6 @@ DINamespaceAttr DebugImporter::translateImpl(llvm::DINamespace *node) {
 DIImportedEntityAttr
 DebugImporter::translateImpl(llvm::DIImportedEntity *node) {
   SmallVector<DINodeAttr> elements;
-
   for (llvm::DINode *element : node->getElements()) {
     assert(element && "expected a non-null element type");
     elements.push_back(translate(element));
@@ -240,9 +239,8 @@ DISubprogramAttr DebugImporter::translateImpl(llvm::DISubprogram *node) {
     return nullptr;
 
   SmallVector<DINodeAttr> retainedNodes;
-
-  for (auto node : node->getRetainedNodes())
-    retainedNodes.push_back(translate(node));
+  for (llvm::DINode *retainedNode : node->getRetainedNodes())
+    retainedNodes.push_back(translate(retainedNode));
 
   return DISubprogramAttr::get(context, id, translate(node->getUnit()), scope,
                                getStringAttrOrNull(node->getRawName()),

mlir/lib/Target/LLVMIR/DebugTranslation.cpp

@@ -306,11 +306,12 @@ llvm::DISubprogram *DebugTranslation::translateImpl(DISubprogramAttr attr) {
       static_cast<llvm::DISubprogram::DISPFlags>(attr.getSubprogramFlags()),
       compileUnit);
 
+  // DIImportedEntity requires scope information which DIImportedEntityAttr does
+  // not have. This is why we translate DIImportedEntityAttr after we have
+  // created DISubprogram as we can use it as the scope.
   SmallVector<llvm::Metadata *> retainedNodes;
-
-  for (auto nodeAttr : attr.getRetainedNodes()) {
-    if (DIImportedEntityAttr importedAttr =
-            dyn_cast<DIImportedEntityAttr>(nodeAttr)) {
+  for (DINodeAttr nodeAttr : attr.getRetainedNodes()) {
+    if (auto importedAttr = dyn_cast<DIImportedEntityAttr>(nodeAttr)) {
       llvm::DINode *dn = translate(importedAttr, node);
       retainedNodes.push_back(dn);
     }

mlir/lib/Target/LLVMIR/DebugTranslation.h

@@ -89,6 +89,11 @@ class DebugTranslation {
   llvm::DISubrange *translateImpl(DISubrangeAttr attr);
   llvm::DISubroutineType *translateImpl(DISubroutineTypeAttr attr);
   llvm::DIType *translateImpl(DITypeAttr attr);
+
+  /// Currently, DIImportedEntityAttr does not have a scope field to avoid a
+  /// cyclic dependency.  The scope information is obtained from the entity
+  /// which holds the list of DIImportedEntityAttr. This requires that scope
+  /// information be passed to translate function.
   llvm::DIImportedEntity *translate(DIImportedEntityAttr attr, llvm::DIScope *);
 
   /// Attributes that support self recursion need to implement an additional