Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[flang][NFC] Moving alias analysis utilities utilities together. Adding new utility. #125925

Merged
merged 2 commits into from
Feb 6, 2025

Conversation

Renaud-K
Copy link
Contributor

@Renaud-K Renaud-K commented Feb 5, 2025

  1. Our static functions are a bit spread out in this file. I am gathering them in an anonymous namespace
  2. Moving the code to get the target attribute on a fir.global into its own utility.

@llvmbot llvmbot added flang Flang issues not falling into any other category flang:fir-hlfir labels Feb 5, 2025
@Renaud-K Renaud-K changed the title Moving utilities into anonymous namespaces. flang] Moving alias analysis utilities into anonymous namespaces. Feb 5, 2025
@llvmbot
Copy link
Member

llvmbot commented Feb 5, 2025

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

Author: Renaud Kauffmann (Renaud-K)

Changes
  1. Our static functions are a bit spread out in this file. I am gathering them in an anonymous namespace
  2. Moving the code to get the target attribute on a fir.global into its own utility.

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

1 Files Affected:

  • (modified) flang/lib/Optimizer/Analysis/AliasAnalysis.cpp (+54-44)
diff --git a/flang/lib/Optimizer/Analysis/AliasAnalysis.cpp b/flang/lib/Optimizer/Analysis/AliasAnalysis.cpp
index e33d8fa333e7a59..63f4881f13c522b 100644
--- a/flang/lib/Optimizer/Analysis/AliasAnalysis.cpp
+++ b/flang/lib/Optimizer/Analysis/AliasAnalysis.cpp
@@ -31,9 +31,29 @@ using namespace mlir;
 // AliasAnalysis: alias
 //===----------------------------------------------------------------------===//
 
-/// Temporary function to skip through all the no op operations
-/// TODO: Generalize support of fir.load
-static mlir::Value getOriginalDef(mlir::Value v) {
+namespace {
+
+fir::AliasAnalysis::Source::Attributes
+getAttrsFromVariable(fir::FortranVariableOpInterface var) {
+  fir::AliasAnalysis::Source::Attributes attrs;
+  if (var.isTarget())
+    attrs.set(fir::AliasAnalysis::Attribute::Target);
+  if (var.isPointer())
+    attrs.set(fir::AliasAnalysis::Attribute::Pointer);
+  if (var.isIntentIn())
+    attrs.set(fir::AliasAnalysis::Attribute::IntentIn);
+
+  return attrs;
+}
+
+bool hasGlobalOpTargetAttr(mlir::Value v, fir::AddrOfOp op) {
+  auto globalOpName =
+      mlir::OperationName(fir::GlobalOp::getOperationName(), op->getContext());
+  return fir::valueHasFirAttribute(
+      v, fir::GlobalOp::getTargetAttrName(globalOpName));
+}
+
+mlir::Value getOriginalDef(mlir::Value v) {
   mlir::Operation *defOp;
   bool breakFromLoop = false;
   while (!breakFromLoop && (defOp = v.getDefiningOp())) {
@@ -46,6 +66,29 @@ static mlir::Value getOriginalDef(mlir::Value v) {
   return v;
 }
 
+bool isEvaluateInMemoryBlockArg(mlir::Value v) {
+  if (auto evalInMem = llvm::dyn_cast_or_null<hlfir::EvaluateInMemoryOp>(
+          v.getParentRegion()->getParentOp()))
+    return evalInMem.getMemory() == v;
+  return false;
+}
+
+template <typename OMPTypeOp, typename DeclTypeOp>
+bool isPrivateArg(omp::BlockArgOpenMPOpInterface &argIface, OMPTypeOp &op,
+                  DeclTypeOp &declOp) {
+  if (!op.getPrivateSyms().has_value())
+    return false;
+  for (auto [opSym, blockArg] :
+       llvm::zip_equal(*op.getPrivateSyms(), argIface.getPrivateBlockArgs())) {
+    if (blockArg == declOp.getMemref()) {
+      return true;
+    }
+  }
+  return false;
+}
+
+} // namespace
+
 namespace fir {
 
 void AliasAnalysis::Source::print(llvm::raw_ostream &os) const {
@@ -91,13 +134,6 @@ bool AliasAnalysis::Source::isDummyArgument() const {
   return false;
 }
 
-static bool isEvaluateInMemoryBlockArg(mlir::Value v) {
-  if (auto evalInMem = llvm::dyn_cast_or_null<hlfir::EvaluateInMemoryOp>(
-          v.getParentRegion()->getParentOp()))
-    return evalInMem.getMemory() == v;
-  return false;
-}
-
 bool AliasAnalysis::Source::isData() const { return origin.isData; }
 bool AliasAnalysis::Source::isBoxData() const {
   return mlir::isa<fir::BaseBoxType>(fir::unwrapRefType(valueType)) &&
@@ -348,7 +384,9 @@ AliasResult AliasAnalysis::alias(Source lhsSrc, Source rhsSrc, mlir::Value lhs,
 // AliasAnalysis: getModRef
 //===----------------------------------------------------------------------===//
 
-static bool isSavedLocal(const fir::AliasAnalysis::Source &src) {
+namespace {
+
+bool isSavedLocal(const fir::AliasAnalysis::Source &src) {
   if (auto symRef = llvm::dyn_cast<mlir::SymbolRefAttr>(src.origin.u)) {
     auto [nameKind, deconstruct] =
         fir::NameUniquer::deconstruct(symRef.getLeafReference().getValue());
@@ -358,7 +396,7 @@ static bool isSavedLocal(const fir::AliasAnalysis::Source &src) {
   return false;
 }
 
-static bool isCallToFortranUserProcedure(fir::CallOp call) {
+bool isCallToFortranUserProcedure(fir::CallOp call) {
   // TODO: indirect calls are excluded by these checks. Maybe some attribute is
   // needed to flag user calls in this case.
   if (fir::hasBindcAttr(call))
@@ -369,7 +407,7 @@ static bool isCallToFortranUserProcedure(fir::CallOp call) {
   return false;
 }
 
-static ModRefResult getCallModRef(fir::CallOp call, mlir::Value var) {
+ModRefResult getCallModRef(fir::CallOp call, mlir::Value var) {
   // TODO: limit to Fortran functions??
   // 1. Detect variables that can be accessed indirectly.
   fir::AliasAnalysis aliasAnalysis;
@@ -423,6 +461,8 @@ static ModRefResult getCallModRef(fir::CallOp call, mlir::Value var) {
   return ModRefResult::getNoModRef();
 }
 
+} // namespace
+
 /// This is mostly inspired by MLIR::LocalAliasAnalysis with 2 notable
 /// differences 1) Regions are not handled here but will be handled by a data
 /// flow analysis to come 2) Allocate and Free effects are considered
@@ -491,33 +531,6 @@ ModRefResult AliasAnalysis::getModRef(mlir::Region &region,
   return result;
 }
 
-AliasAnalysis::Source::Attributes
-getAttrsFromVariable(fir::FortranVariableOpInterface var) {
-  AliasAnalysis::Source::Attributes attrs;
-  if (var.isTarget())
-    attrs.set(AliasAnalysis::Attribute::Target);
-  if (var.isPointer())
-    attrs.set(AliasAnalysis::Attribute::Pointer);
-  if (var.isIntentIn())
-    attrs.set(AliasAnalysis::Attribute::IntentIn);
-
-  return attrs;
-}
-
-template <typename OMPTypeOp, typename DeclTypeOp>
-static bool isPrivateArg(omp::BlockArgOpenMPOpInterface &argIface,
-                         OMPTypeOp &op, DeclTypeOp &declOp) {
-  if (!op.getPrivateSyms().has_value())
-    return false;
-  for (auto [opSym, blockArg] :
-       llvm::zip_equal(*op.getPrivateSyms(), argIface.getPrivateBlockArgs())) {
-    if (blockArg == declOp.getMemref()) {
-      return true;
-    }
-  }
-  return false;
-}
-
 AliasAnalysis::Source AliasAnalysis::getSource(mlir::Value v,
                                                bool getLastInstantiationPoint) {
   auto *defOp = v.getDefiningOp();
@@ -604,10 +617,7 @@ AliasAnalysis::Source AliasAnalysis::getSource(mlir::Value v,
           ty = v.getType();
           type = SourceKind::Global;
 
-          auto globalOpName = mlir::OperationName(
-              fir::GlobalOp::getOperationName(), defOp->getContext());
-          if (fir::valueHasFirAttribute(
-                  v, fir::GlobalOp::getTargetAttrName(globalOpName)))
+          if (hasGlobalOpTargetAttr(v, op))
             attributes.set(Attribute::Target);
 
           // TODO: Take followBoxData into account when setting the pointer

@tblah
Copy link
Contributor

tblah commented Feb 6, 2025

The LLVM coding standards say the following:

we have a simple guideline: make anonymous namespaces as small as possible, and only use them for class declarations.

@jeanPerier
Copy link
Contributor

I agree with @tblah, this change is not in line with the coding style I am seeing in LLVM.

Copy link
Collaborator

@jdenny-ornl jdenny-ornl left a comment

Choose a reason for hiding this comment

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

LGTM. Please add NFC to the commit title. Please update the commit log for the use of static.

@Renaud-K Renaud-K changed the title flang] Moving alias analysis utilities into anonymous namespaces. [flang][NFC] Moving alias analysis utilities into anonymous namespaces. Feb 6, 2025
@Renaud-K Renaud-K changed the title [flang][NFC] Moving alias analysis utilities into anonymous namespaces. [flang][NFC] Moving alias analysis utilities utilities together. Adding new utility. Feb 6, 2025
@Renaud-K Renaud-K merged commit 6dc41a6 into llvm:main Feb 6, 2025
8 checks passed
@jeanPerier
Copy link
Contributor

LGTM, thanks

Icohedron pushed a commit to Icohedron/llvm-project that referenced this pull request Feb 11, 2025
…ng new utility. (llvm#125925)

1. Our static functions are a bit spread out in this file. I am
gathering them in an anonymous namespace
2. Moving the code to get the `target` attribute on a `fir.global` into
its own utility.
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.

5 participants