[flang] [NFCI] Using getSource instead of getOriginalDef #128984

Renaud-K · 2025-02-27T02:09:46Z

As discussed in past MRs, this change removes the use of getOriginalDef to use getSource instead to gather data from an indirection.

llvmbot · 2025-02-27T02:10:19Z

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

Author: Renaud Kauffmann (Renaud-K)

Changes

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

1 Files Affected:

(modified) flang/lib/Optimizer/Analysis/AliasAnalysis.cpp (+34-29)

diff --git a/flang/lib/Optimizer/Analysis/AliasAnalysis.cpp b/flang/lib/Optimizer/Analysis/AliasAnalysis.cpp
index 2bc79100b03b6..5ce5a9d0c0dd7 100644
--- a/flang/lib/Optimizer/Analysis/AliasAnalysis.cpp
+++ b/flang/lib/Optimizer/Analysis/AliasAnalysis.cpp
@@ -439,11 +439,16 @@ static ModRefResult getCallModRef(fir::CallOp call, mlir::Value var) {
   // An example of something currently problematic is the allocmem generated for
   // ALLOCATE of allocatable target. It currently does not have the target
   // attribute, which would lead this analysis to believe it cannot escape.
-  if (!varSrc.isFortranUserVariable() || !isCallToFortranUserProcedure(call))
+  if (!varSrc.isFortranUserVariable() || !isCallToFortranUserProcedure(call)) {
+    LLVM_DEBUG(llvm::dbgs() << "ModAndRef because not a user variable or not a "
+                               "call to user procedure.\n");
     return ModRefResult::getModAndRef();
+  }
   // Pointer and target may have been captured.
-  if (varSrc.isTargetOrPointer())
+  if (varSrc.isTargetOrPointer()) {
+    LLVM_DEBUG(llvm::dbgs() << "ModAndRef because target or pointer.\n");
     return ModRefResult::getModAndRef();
+  }
   // Host associated variables may be addressed indirectly via an internal
   // function call, whether the call is in the parent or an internal procedure.
   // Note that the host associated/internal procedure may be referenced
@@ -451,8 +456,11 @@ static ModRefResult getCallModRef(fir::CallOp call, mlir::Value var) {
   // procedures may be captured or passed. As this is tricky to analyze, always
   // consider such variables may be accessed in any calls.
   if (varSrc.kind == fir::AliasAnalysis::SourceKind::HostAssoc ||
-      varSrc.isCapturedInInternalProcedure)
+      varSrc.isCapturedInInternalProcedure) {
+    LLVM_DEBUG(llvm::dbgs() << "ModAndRef because var is host associated or "
+                               "captured in internal procedure.\n");
     return ModRefResult::getModAndRef();
+  }
   // At that stage, it has been ruled out that local (including the saved ones)
   // and dummy cannot be indirectly accessed in the call.
   if (varSrc.kind != fir::AliasAnalysis::SourceKind::Allocate &&
@@ -485,6 +493,8 @@ ModRefResult AliasAnalysis::getModRef(Operation *op, Value location) {
   if (!interface) {
     if (auto call = llvm::dyn_cast<fir::CallOp>(op))
       return getCallModRef(call, location);
+
+    LLVM_DEBUG(llvm::dbgs() << "ModAndRef because no interface or call.\n");
     return ModRefResult::getModAndRef();
   }
 
@@ -621,38 +631,33 @@ AliasAnalysis::Source AliasAnalysis::getSource(mlir::Value v,
             if (mlir::isa<fir::PointerType>(boxTy.getEleTy()))
               attributes.set(Attribute::Pointer);
 
-            auto def = getOriginalDef(op.getMemref(), attributes,
-                                      isCapturedInInternalProcedure,
-                                      approximateSource);
-            if (auto addrOfOp = def.template getDefiningOp<fir::AddrOfOp>()) {
-              global = addrOfOp.getSymbol();
-
-              if (hasGlobalOpTargetAttr(def, addrOfOp))
-                attributes.set(Attribute::Target);
+            auto boxSrc = getSource(op.getMemref());
+            attributes |= boxSrc.attributes;
+            approximateSource |= boxSrc.approximateSource;
+            isCapturedInInternalProcedure |= boxSrc.isCapturedInInternalProcedure;
 
+            global = llvm::dyn_cast<mlir::SymbolRefAttr>(boxSrc.origin.u);
+            if (global) {
               type = SourceKind::Global;
-            }
-            // TODO: Add support to fir.allocmem
-            else if (auto allocOp =
-                         def.template getDefiningOp<fir::AllocaOp>()) {
-              v = def;
-              defOp = v.getDefiningOp();
-              type = SourceKind::Allocate;
-            } else if (isDummyArgument(def)) {
-              defOp = nullptr;
-              v = def;
             } else {
-              type = SourceKind::Indirect;
+              auto def = llvm::cast<mlir::Value>(boxSrc.origin.u);
+              // TODO: Add support to fir.allocmem
+              if (auto allocOp = def.template getDefiningOp<fir::AllocaOp>()) {
+                v = def;
+                defOp = v.getDefiningOp();
+                type = SourceKind::Allocate;
+              } else if (isDummyArgument(def)) {
+                defOp = nullptr;
+                v = def;
+              } else {
+                type = SourceKind::Indirect;
+              }
             }
-            // TODO: This assignment is redundant but somehow works around an
-            // apparent MSVC bug reporting "undeclared identifier" at the next
-            // "breakFromLoop = true;".  See
-            // <https://github.com/llvm/llvm-project/pull/127845#issuecomment-2669829610>.
             breakFromLoop = true;
-          } else {
-            // No further tracking for addresses loaded from memory for now.
-            type = SourceKind::Indirect;
+            return;
           }
+          // No further tracking for addresses loaded from memory for now.
+          type = SourceKind::Indirect;
           breakFromLoop = true;
         })
         .Case<fir::AddrOfOp>([&](auto op) {

github-actions · 2025-02-27T02:13:01Z

✅ With the latest revision this PR passed the C/C++ code formatter.

jeanPerier

Thanks for following up on our discussion!

jeanPerier · 2025-02-28T15:59:38Z

flang/lib/Optimizer/Analysis/AliasAnalysis.cpp

+              // TODO: Add support to fir.allocmem
+              if (auto allocOp = def.template getDefiningOp<fir::AllocaOp>()) {


Why not using the boxSrc SourceKind?

If the attributes are merged and the sourceKind come boxSrc, how much different would it be to just follow the source? :) We would be back to square one.

This is why I want to ultimately introduce SourceKind::Emboxed. It is an indirection for which we are comfortable applying static attributes to make an aliasing determination. While, for other type of indirection, lowering cannot guarantee that an alias situation is always indicated with a fir.ptr type for instance.

Yes, I was not suggesting using it in general, I just do not see how if (auto allocOp = def.template getDefiningOp<fir::AllocaOp>()) + the TODO about allocmem is very different from if (boxSrc.kind == SourceKind::Allocate)

Anyway, as mention below, I share your concern about the propagation of the source kind here, and agree the source kind should either remain Indirect + extra guarantee info (namely, if this is an ALLOCATABLE/POINTER or unknown/compiler generated indirection).

jeanPerier · 2025-02-28T16:02:48Z

flang/lib/Optimizer/Analysis/AliasAnalysis.cpp

+              if (auto allocOp = def.template getDefiningOp<fir::AllocaOp>()) {
+                v = def;
+                defOp = v.getDefiningOp();
+                type = SourceKind::Allocate;


I do not really get why we can propagate the source kind here. The pointer storage may be local, but the address inside the pointer could very well point to some global or dummy memory.

I know this was already the case, so I am OK with the change, but I think we may need to discuss/clarify this.

I raised that concern as well. But we do not have a proper way of modeling this as @jdenny-ornl showed here. I do get more meaningful results with the SourceKind::Emboxed here as well.

Are you concerned specifically for a pointer that is an AllocaOp (locals)? We handle a pointer that is an AddrOfOp (globals) or dummy arg in the same manner: for the address loaded from that pointer, the type is Allocate, Global, or Argument, respectively, even though it might dynamically be the address of some other type.

Previously, we instead set type=Direct for an address loaded from a pointer that is AddrOfOp. I attempted to do the same for dummy args and planned to do the same for AllocaOp, but then we got rid of Direct.

At this point, I haven't tried to figure out whether using Direct would have worked out for all use cases we have discussed since then. Reviewing the rationale for dropping Direct might be worthwhile if you are thinking of introducing another type to serve a role similar to the direction I was heading with Direct.

Direct was a misnomer because you never know what was stored at the address loaded. We can still use aliasing rules on these box indirections because we are comfortable relying on source level attributes (POINTER, TARGET,...). We assume that FIR will not introduce an aliasing situation without these attributes.

FIR does not currently guarantee this for scalars. This distinction only became clear to me recently. So Emboxed would be more precise. If and when we can treat scalars like boxes, we can regroup Emboxed and Indirect and treat them the same.

Direct was a misnomer

I agree it wasn't the right name for the direction I was taking it.

jeanPerier

Thanks

jdenny-ornl · 2025-03-04T17:53:28Z

flang/lib/Optimizer/Analysis/AliasAnalysis.cpp

-
-              if (hasGlobalOpTargetAttr(def, addrOfOp))
-                attributes.set(Attribute::Target);
+            auto boxSrc = getSource(op.getMemref());


What ensures this getSource call will not recurse through a second LoadOp case? getOriginalDef clearly could not do that.

It is the drawback and the reason why I preferred getOrginalDef.
Though, since it is not following data, it should return an indirect source kind and the load op as the source.
I could add a test, I guess.

I think it would also be helpful to have a brief comment on this getSource call: This getSource call will never recurse through a second LoadOp because followData will necessarily become false within the call and thus the above boxTy && followData guard will prevent this block from executing again.

But is that true for the part of the LoadOp case outside that block: the omp::TargetOp handling? Recursing to that from this getSource call was not possible with getOriginalDef, but I'm not sure now.

If we want to be more confident that this PR is NFC, we could add a mode parameter for getSource that causes it to stop with Indirect on any case that getOriginalDef didn't handle. We could then gradually extend the implementation to eliminate those Indirect cases in later PRs as driven by real use cases. That approach seems to achieve a reasonable balance with the maintenance rationale for eliminating getOriginalDef.

As discussed in past MRs, this change removes the use of getOriginalDef to use getSource instead to gather data from an indirection.

llvmbot added flang Flang issues not falling into any other category flang:fir-hlfir labels Feb 27, 2025

Using getSource instead of getOriginalDef

ce4bf77

Renaud-K force-pushed the alias-analysis-get-source branch from 47e8d7f to ce4bf77 Compare February 27, 2025 17:26

Renaud-K changed the title ~~[WIP] Using getSource instead of getOriginalDef~~ Using getSource instead of getOriginalDef Feb 27, 2025

Renaud-K requested review from jdenny-ornl, tblah and jeanPerier February 27, 2025 17:53

Renaud-K changed the title ~~Using getSource instead of getOriginalDef~~ [flang] [NFCI] Using getSource instead of getOriginalDef Feb 27, 2025

jeanPerier reviewed Feb 28, 2025

View reviewed changes

jeanPerier approved these changes Mar 4, 2025

View reviewed changes

jdenny-ornl reviewed Mar 4, 2025

View reviewed changes

Adding source kind test

4474017

Renaud-K merged commit 718c4ed into llvm:main Mar 7, 2025
11 checks passed

		// TODO: Add support to fir.allocmem
		if (auto allocOp = def.template getDefiningOp<fir::AllocaOp>()) {

[flang] [NFCI] Using getSource instead of getOriginalDef #128984

[flang] [NFCI] Using getSource instead of getOriginalDef #128984

Uh oh!

Conversation

Renaud-K commented Feb 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Feb 27, 2025

Uh oh!

github-actions bot commented Feb 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jeanPerier left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Renaud-K Feb 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeanPerier left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Renaud-K commented Feb 27, 2025 •

edited

Loading

github-actions bot commented Feb 27, 2025 •

edited

Loading

Renaud-K Feb 28, 2025 •

edited

Loading