Skip to content

Conversation

@abidh
Copy link
Contributor

@abidh abidh commented Dec 2, 2024

When a global variable is used in the OpenMP target region, it is passed as an argument to the function that implements target region. But the DeclareOp for this incarnation still have the original name of the variable. As some of our checks to decide if a variable is global or nor are based on the name, this can result in a local variable being treated as global. This PR hardens the check a bit. We now also check that memory ref is actually an AddrOfOp before looking at the name.

When a global variable is used in the OpenMP target region, it is
passed as an argument to the function that implements target region.
But the DeclareOp for this incarnation still have the original name of
the variable. As some of our checks to decide if a variable is global
or nor are based on the name, this can result in a local variable being
treated as global. This PR hardens the check a bit. We now also check
that memory ref is actually `an AddrOfOp` before looking at the name.
@llvmbot llvmbot added flang Flang issues not falling into any other category flang:fir-hlfir labels Dec 2, 2024
@llvmbot
Copy link
Member

llvmbot commented Dec 2, 2024

@llvm/pr-subscribers-flang-fir-hlfir

Author: Abid Qadeer (abidh)

Changes

When a global variable is used in the OpenMP target region, it is passed as an argument to the function that implements target region. But the DeclareOp for this incarnation still have the original name of the variable. As some of our checks to decide if a variable is global or nor are based on the name, this can result in a local variable being treated as global. This PR hardens the check a bit. We now also check that memory ref is actually an AddrOfOp before looking at the name.


Full diff: https://github.com/llvm/llvm-project/pull/118326.diff

1 Files Affected:

  • (modified) flang/lib/Optimizer/Transforms/AddDebugInfo.cpp (+7-7)
diff --git a/flang/lib/Optimizer/Transforms/AddDebugInfo.cpp b/flang/lib/Optimizer/Transforms/AddDebugInfo.cpp
index 3a437c7a0f0137..db223e99b6c59c 100644
--- a/flang/lib/Optimizer/Transforms/AddDebugInfo.cpp
+++ b/flang/lib/Optimizer/Transforms/AddDebugInfo.cpp
@@ -102,15 +102,15 @@ void AddDebugInfoPass::handleDeclareOp(fir::cg::XDeclareOp declOp,
   if (result.first != fir::NameUniquer::NameKind::VARIABLE)
     return;
   // If this DeclareOp actually represents a global then treat it as such.
-  if (auto global = symbolTable->lookup<fir::GlobalOp>(declOp.getUniqName())) {
-    handleGlobalOp(global, fileAttr, scopeAttr, typeGen, symbolTable, declOp);
-    return;
+  mlir::Operation *defOp = declOp.getMemref().getDefiningOp();
+  if (defOp && llvm::isa<fir::AddrOfOp>(defOp)) {
+    if (auto global =
+            symbolTable->lookup<fir::GlobalOp>(declOp.getUniqName())) {
+      handleGlobalOp(global, fileAttr, scopeAttr, typeGen, symbolTable, declOp);
+      return;
+    }
   }
 
-  // Only accept local variables.
-  if (result.second.procs.empty())
-    return;
-
   // FIXME: There may be cases where an argument is processed a bit before
   // DeclareOp is generated. In that case, DeclareOp may point to an
   // intermediate op and not to BlockArgument.

Copy link
Contributor

@jeanPerier jeanPerier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I do not know much about debugging inside OpenMP contexts though.

abidh added a commit to abidh/llvm-project that referenced this pull request Jan 14, 2025
@abidh abidh merged commit 7ece824 into llvm:main Feb 4, 2025
11 checks passed
abidh added a commit to abidh/llvm-project that referenced this pull request Feb 4, 2025
Icohedron pushed a commit to Icohedron/llvm-project that referenced this pull request Feb 11, 2025
)

When a global variable is used in the OpenMP target region, it is passed
as an argument to the function that implements target region. But the
`DeclareOp` for this incarnation still have the original name of the
variable. As some of our checks to decide if a variable is global or nor
are based on the name, this can result in a local variable being treated
as global. This PR hardens the check a bit. We now also check that
memory ref is actually an `AddrOfOp` before looking at the name.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

flang:fir-hlfir flang Flang issues not falling into any other category

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants