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] AliasAnalysis: More formally define and distinguish between data and non-data #91020

Merged
merged 7 commits into from
May 16, 2024

Conversation

Renaud-K
Copy link
Contributor

@Renaud-K Renaud-K commented May 3, 2024

This PR is an implementation for changes proposed in https://discourse.llvm.org/t/rfc-distinguish-between-data-and-non-data-in-fir-alias-analysis/78759

Test updates were made when the query was on the wrong reference. So, it is my hope that this will clear ambiguity on the nature of the queries from here on.
There are also some TODOs that were addressed.

It also partly implements what #87723 is attempting to accomplish. At least, on a point-to-point query between references, the distinction is made. To apply it to TBAA, would be another PR.

Note that, the changes were minimal in the TBAA code to retain the current results.

Copy link
Contributor

@vzakhari vzakhari left a comment

Choose a reason for hiding this comment

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

Thank you for the changes, Renaud! It looks clearer to me now. I have a question about the TBAA tags pass.

flang/lib/Optimizer/Transforms/AddAliasTags.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@tblah tblah left a comment

Choose a reason for hiding this comment

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

Thanks for this, it gives a big improvement to readability and it is good to catch more cases.

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.

Thank you @Renaud-K , it is great to simplify the logic here! I think you need to be more restrictive with types containing pointer/allocatables components (I see there are already some restrictions using isRecordWithPointerComponent, but they do not cover some previous early return cases).

flang/lib/Optimizer/Analysis/AliasAnalysis.cpp Outdated Show resolved Hide resolved
flang/lib/Optimizer/Analysis/AliasAnalysis.cpp Outdated Show resolved Hide resolved
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.

Thanks for working on this.

@llvmbot llvmbot added flang Flang issues not falling into any other category flang:fir-hlfir labels May 7, 2024
@llvmbot
Copy link
Member

llvmbot commented May 7, 2024

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

Author: Renaud Kauffmann (Renaud-K)

Changes

This PR is an implementation for changes proposed in https://discourse.llvm.org/t/rfc-distinguish-between-data-and-non-data-in-fir-alias-analysis/78759

Test updates were made when the query was on the wrong reference. So, it is my hope that this will clear ambiguity on the nature of the queries from here on.
There are also some TODOs that were addressed.

It also partly implements what #87723 is attempting to accomplish. At least, on a point-to-point query between references, the distinction is made. To apply it to TBAA, would be another PR.

Note that, the changes were minimal in the TBAA code to retain the current results.


Patch is 29.71 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/91020.diff

6 Files Affected:

  • (modified) flang/include/flang/Optimizer/Analysis/AliasAnalysis.h (+75-5)
  • (modified) flang/lib/Optimizer/Analysis/AliasAnalysis.cpp (+63-90)
  • (modified) flang/lib/Optimizer/Transforms/AddAliasTags.cpp (+10-7)
  • (modified) flang/test/Analysis/AliasAnalysis/alias-analysis-2.fir (+24-13)
  • (modified) flang/test/Analysis/AliasAnalysis/alias-analysis-3.fir (+2-2)
  • (added) flang/test/Analysis/AliasAnalysis/alias-analysis-9.fir (+51)
diff --git a/flang/include/flang/Optimizer/Analysis/AliasAnalysis.h b/flang/include/flang/Optimizer/Analysis/AliasAnalysis.h
index dfcafe88fee1b5..8a0eb525f19157 100644
--- a/flang/include/flang/Optimizer/Analysis/AliasAnalysis.h
+++ b/flang/include/flang/Optimizer/Analysis/AliasAnalysis.h
@@ -36,9 +36,6 @@ struct AliasAnalysis {
              /// Represents memory allocated outside of a function
              /// and passed to the function via host association tuple.
              HostAssoc,
-             /// Represents direct memory access whose source cannot be further
-             /// determined
-             Direct,
              /// Represents memory allocated by unknown means and
              /// with the memory address defined by a memory reading
              /// operation (e.g. fir::LoadOp).
@@ -50,12 +47,70 @@ struct AliasAnalysis {
   /// Attributes of the memory source object.
   ENUM_CLASS(Attribute, Target, Pointer, IntentIn);
 
+  // See
+  // https://discourse.llvm.org/t/rfc-distinguish-between-data-and-non-data-in-fir-alias-analysis/78759/1
+  //
+  // It is possible, while following the source of a memory reference through
+  // the use-def chain, to arrive at the same origin, even though the starting
+  // points were known to not alias.
+  //
+  // Example:
+  // clang-format off
+  //    fir.global @_QMtopEa : !fir.box<!fir.ptr<!fir.array<?xf32>>> 
+  //  
+  //  func.func @_QPtest() {
+  //    %c1 = arith.constant 1 : index
+  //    %cst = arith.constant 1.000000e+00 : f32
+  //    %0 = fir.address_of(@_QMtopEa) : !fir.ref<!fir.box<!fir.ptr<!fir.array<?xf32>>>>
+  //    %1 = fir.declare %0 {fortran_attrs = #fir.var_attrs<pointer>, uniq_name = "_QMtopEa"} : (!fir.ref<!fir.box<!fir.ptr<!fir.array<?xf32>>>>) -> !fir.ref<!fir.box<!fir.ptr<!fir.array<?xf32>>>>
+  //    %2 = fir.load %1 : !fir.ref<!fir.box<!fir.ptr<!fir.array<?xf32>>>>
+  //    ...
+  //    %5 = fir.array_coor %2 %c1 : (!fir.box<!fir.ptr<!fir.array<?xf32>>>, !fir.shift<1>, index) -> !fir.ref<f32>
+  //    fir.store %cst to %5 : !fir.ref<f32>
+  //    return
+  //  }
+  //
+  // With high level operations, such as fir.array_coor, it is possible to
+  // reach into the data wrapped by the box (the descriptor) therefore when
+  // asking about the memory source of the %5, we are really asking about the
+  // source of the data of box %2.
+  //
+  // When asking about the source of %0 which is the address of the box, we
+  // reach the same source as in the first case: the global @_QMtopEa. Yet one
+  // source refers to the data while the other refers to the address of the box
+  // itself.
+  //
+  // To distinguish between the two, the isData flag has been added, whereby
+  // data is defined as any memory reference that is not a box reference.
+  // Additionally, because it is relied on in HLFIR lowering, we allow querying
+  // on a box SSA value, which is interpreted as querying on its data.
+  //
+  // So in the above example, !fir.ref<f32> and !fir.box<!fir.ptr<!fir.array<?xf32>>> is data, 
+  // while !fir.ref<!fir.box<!fir.ptr<!fir.array<?xf32>>>> is not data.
+
+  // This generally applies to function arguments. In the example below, %arg0
+  // is data, %arg1 is not data but a load of %arg1 is.
+  // 
+  // func.func @_QFPtest2(%arg0: !fir.ref<f32>, %arg1: !fir.ref<!fir.box<!fir.ptr<f32>>> )  {
+  //    %0 = fir.load %arg1 : !fir.ref<!fir.box<!fir.ptr<f32>>>
+  //    ... }
+  //
+  // clang-format on
+
   struct Source {
     using SourceUnion = llvm::PointerUnion<mlir::SymbolRefAttr, mlir::Value>;
     using Attributes = Fortran::common::EnumSet<Attribute, Attribute_enumSize>;
 
-    /// Source definition of a value.
-    SourceUnion u;
+    struct SourceOrigin {
+      /// Source definition of a value.
+      SourceUnion u;
+
+      /// Whether the source was reached following data or box reference
+      bool isData{false};
+    };
+
+    SourceOrigin origin;
+
     /// Kind of the memory source.
     SourceKind kind;
     /// Value type of the source definition.
@@ -77,6 +132,12 @@ struct AliasAnalysis {
     /// attribute.
     bool isRecordWithPointerComponent() const;
 
+    bool isDummyArgument() const;
+    bool isData() const;
+    bool isBoxData() const;
+
+    mlir::Type getType() const;
+
     /// Return true, if `ty` is a reference type to a boxed
     /// POINTER object or a raw fir::PointerType.
     static bool isPointerReference(mlir::Type ty);
@@ -95,6 +156,15 @@ struct AliasAnalysis {
   Source getSource(mlir::Value);
 };
 
+inline bool operator==(const AliasAnalysis::Source::SourceOrigin &lhs,
+                       const AliasAnalysis::Source::SourceOrigin &rhs) {
+  return lhs.u == rhs.u && lhs.isData == rhs.isData;
+}
+inline bool operator!=(const AliasAnalysis::Source::SourceOrigin &lhs,
+                       const AliasAnalysis::Source::SourceOrigin &rhs) {
+  return !(lhs == rhs);
+}
+
 inline llvm::raw_ostream &operator<<(llvm::raw_ostream &os,
                                      const AliasAnalysis::Source &op) {
   op.print(os);
diff --git a/flang/lib/Optimizer/Analysis/AliasAnalysis.cpp b/flang/lib/Optimizer/Analysis/AliasAnalysis.cpp
index f723e8f66e3e4b..ade240bc2364e4 100644
--- a/flang/lib/Optimizer/Analysis/AliasAnalysis.cpp
+++ b/flang/lib/Optimizer/Analysis/AliasAnalysis.cpp
@@ -33,7 +33,9 @@ static bool isDummyArgument(mlir::Value v) {
   if (!blockArg)
     return false;
 
-  return blockArg.getOwner()->isEntryBlock();
+  auto *owner{blockArg.getOwner()};
+  return owner->isEntryBlock() &&
+         mlir::isa<mlir::FunctionOpInterface>(owner->getParentOp());
 }
 
 /// Temporary function to skip through all the no op operations
@@ -54,12 +56,17 @@ static mlir::Value getOriginalDef(mlir::Value v) {
 namespace fir {
 
 void AliasAnalysis::Source::print(llvm::raw_ostream &os) const {
-  if (auto v = llvm::dyn_cast<mlir::Value>(u))
+  if (auto v = llvm::dyn_cast<mlir::Value>(origin.u))
     os << v;
-  else if (auto gbl = llvm::dyn_cast<mlir::SymbolRefAttr>(u))
+  else if (auto gbl = llvm::dyn_cast<mlir::SymbolRefAttr>(origin.u))
     os << gbl;
   os << " SourceKind: " << EnumToString(kind);
   os << " Type: " << valueType << " ";
+  if (origin.isData) {
+    os << " following data ";
+  } else {
+    os << " following box reference ";
+  }
   attributes.Dump(os, EnumToString);
 }
 
@@ -76,6 +83,19 @@ bool AliasAnalysis::Source::isTargetOrPointer() const {
          attributes.test(Attribute::Target);
 }
 
+bool AliasAnalysis::Source::isDummyArgument() const {
+  if (auto v = origin.u.dyn_cast<mlir::Value>()) {
+    return ::isDummyArgument(v);
+  }
+  return false;
+}
+
+bool AliasAnalysis::Source::isData() const { return origin.isData; }
+bool AliasAnalysis::Source::isBoxData() const {
+  return mlir::isa<fir::BaseBoxType>(fir::unwrapRefType(valueType)) &&
+         origin.isData;
+}
+
 bool AliasAnalysis::Source::isRecordWithPointerComponent() const {
   auto eleTy = fir::dyn_cast_ptrEleTy(valueType);
   if (!eleTy)
@@ -88,70 +108,35 @@ AliasResult AliasAnalysis::alias(Value lhs, Value rhs) {
   auto lhsSrc = getSource(lhs);
   auto rhsSrc = getSource(rhs);
   bool approximateSource = lhsSrc.approximateSource || rhsSrc.approximateSource;
-  LLVM_DEBUG(llvm::dbgs() << "AliasAnalysis::alias\n";
+  LLVM_DEBUG(llvm::dbgs() << "\n"; llvm::dbgs() << "AliasAnalysis::alias\n";
              llvm::dbgs() << "  lhs: " << lhs << "\n";
              llvm::dbgs() << "  lhsSrc: " << lhsSrc << "\n";
              llvm::dbgs() << "  rhs: " << rhs << "\n";
-             llvm::dbgs() << "  rhsSrc: " << rhsSrc << "\n";
-             llvm::dbgs() << "\n";);
+             llvm::dbgs() << "  rhsSrc: " << rhsSrc << "\n";);
 
   // Indirect case currently not handled. Conservatively assume
   // it aliases with everything
-  if (lhsSrc.kind > SourceKind::Direct || rhsSrc.kind > SourceKind::Direct) {
+  if (lhsSrc.kind >= SourceKind::Indirect ||
+      rhsSrc.kind >= SourceKind::Indirect) {
     return AliasResult::MayAlias;
   }
 
-  // SourceKind::Direct is set for the addresses wrapped in a global boxes.
-  // ie: fir.global @_QMpointersEp : !fir.box<!fir.ptr<f32>>
-  // Though nothing is known about them, they would only alias with targets or
-  // pointers
-  bool directSourceToNonTargetOrPointer = false;
-  if (lhsSrc.u != rhsSrc.u || lhsSrc.kind != rhsSrc.kind) {
-    if ((lhsSrc.kind == SourceKind::Direct && !rhsSrc.isTargetOrPointer()) ||
-        (rhsSrc.kind == SourceKind::Direct && !lhsSrc.isTargetOrPointer()))
-      directSourceToNonTargetOrPointer = true;
-  }
-
-  if (lhsSrc.kind == SourceKind::Direct ||
-      rhsSrc.kind == SourceKind::Direct) {
-    if (!directSourceToNonTargetOrPointer)
-      return AliasResult::MayAlias;
-  }
-
   if (lhsSrc.kind == rhsSrc.kind) {
-    if (lhsSrc.u == rhsSrc.u) {
+    if (lhsSrc.origin == rhsSrc.origin) {
+      LLVM_DEBUG(llvm::dbgs()
+                 << "  aliasing because same source kind and origin\n");
       if (approximateSource)
         return AliasResult::MayAlias;
       return AliasResult::MustAlias;
     }
 
     // Two host associated accesses may overlap due to an equivalence.
-    if (lhsSrc.kind == SourceKind::HostAssoc)
-      return AliasResult::MayAlias;
-
-    // Allocate and global memory address cannot physically alias
-    if (lhsSrc.kind == SourceKind::Allocate ||
-        lhsSrc.kind == SourceKind::Global)
-      return AliasResult::NoAlias;
-
-    // Dummy TARGET/POINTER arguments may alias.
-    if (lhsSrc.isTargetOrPointer() && rhsSrc.isTargetOrPointer())
-      return AliasResult::MayAlias;
-
-    // Box for POINTER component inside an object of a derived type
-    // may alias box of a POINTER object, as well as boxes for POINTER
-    // components inside two objects of derived types may alias.
-    if ((lhsSrc.isRecordWithPointerComponent() && rhsSrc.isTargetOrPointer()) ||
-        (rhsSrc.isRecordWithPointerComponent() && lhsSrc.isTargetOrPointer()) ||
-        (lhsSrc.isRecordWithPointerComponent() &&
-         rhsSrc.isRecordWithPointerComponent()))
+    if (lhsSrc.kind == SourceKind::HostAssoc) {
+      LLVM_DEBUG(llvm::dbgs() << "  aliasing because of host association\n");
       return AliasResult::MayAlias;
-
-    return AliasResult::NoAlias;
+    }
   }
 
-  assert(lhsSrc.kind != rhsSrc.kind && "memory source kinds must be different");
-
   Source *src1, *src2;
   if (lhsSrc.kind < rhsSrc.kind) {
     src1 = &lhsSrc;
@@ -182,8 +167,11 @@ AliasResult AliasAnalysis::alias(Value lhs, Value rhs) {
   }
 
   // Dummy TARGET/POINTER argument may alias with a global TARGET/POINTER.
-  if (src1->isTargetOrPointer() && src2->isTargetOrPointer())
+  if (src1->isTargetOrPointer() && src2->isTargetOrPointer() &&
+      src1->isData() == src2->isData()) {
+    LLVM_DEBUG(llvm::dbgs() << "  aliasing because of target or pointer\n");
     return AliasResult::MayAlias;
+  }
 
   // Box for POINTER component inside an object of a derived type
   // may alias box of a POINTER object, as well as boxes for POINTER
@@ -191,8 +179,10 @@ AliasResult AliasAnalysis::alias(Value lhs, Value rhs) {
   if ((src1->isRecordWithPointerComponent() && src2->isTargetOrPointer()) ||
       (src2->isRecordWithPointerComponent() && src1->isTargetOrPointer()) ||
       (src1->isRecordWithPointerComponent() &&
-       src2->isRecordWithPointerComponent()))
+       src2->isRecordWithPointerComponent())) {
+    LLVM_DEBUG(llvm::dbgs() << "  aliasing because of pointer components\n");
     return AliasResult::MayAlias;
+  }
 
   return AliasResult::NoAlias;
 }
@@ -258,7 +248,10 @@ AliasAnalysis::Source AliasAnalysis::getSource(mlir::Value v) {
   mlir::Type ty;
   bool breakFromLoop{false};
   bool approximateSource{false};
-  bool followBoxAddr{mlir::isa<fir::BaseBoxType>(v.getType())};
+  bool followBoxData{mlir::isa<fir::BaseBoxType>(v.getType())};
+  bool isBoxRef{fir::isa_ref_type(v.getType()) &&
+                mlir::isa<fir::BaseBoxType>(fir::unwrapRefType(v.getType()))};
+  bool followingData = !isBoxRef || followBoxData;
   mlir::SymbolRefAttr global;
   Source::Attributes attributes;
   while (defOp && !breakFromLoop) {
@@ -278,34 +271,32 @@ AliasAnalysis::Source AliasAnalysis::getSource(mlir::Value v) {
           v = op->getOperand(0);
           defOp = v.getDefiningOp();
           if (mlir::isa<fir::BaseBoxType>(v.getType()))
-            followBoxAddr = true;
+            followBoxData = true;
         })
         .Case<fir::ArrayCoorOp, fir::CoordinateOp>([&](auto op) {
           v = op->getOperand(0);
           defOp = v.getDefiningOp();
           if (mlir::isa<fir::BaseBoxType>(v.getType()))
-            followBoxAddr = true;
+            followBoxData = true;
           approximateSource = true;
         })
         .Case<fir::EmboxOp, fir::ReboxOp>([&](auto op) {
-          if (followBoxAddr) {
+          if (followBoxData) {
             v = op->getOperand(0);
             defOp = v.getDefiningOp();
           } else
             breakFromLoop = true;
         })
         .Case<fir::LoadOp>([&](auto op) {
-          if (followBoxAddr && mlir::isa<fir::BaseBoxType>(op.getType())) {
-            // For now, support the load of an argument or fir.address_of
-            // TODO: generalize to all operations (in particular fir.alloca and
-            // fir.allocmem)
-            auto def = getOriginalDef(op.getMemref());
-            if (isDummyArgument(def) ||
-                def.template getDefiningOp<fir::AddrOfOp>()) {
-              v = def;
-              defOp = v.getDefiningOp();
-              return;
-            }
+          // If the load is from a leaf source, return the leaf. Do not track
+          // through indirections otherwise.
+          // TODO: At support to fir.alloca and fir.allocmem
+          auto def = getOriginalDef(op.getMemref());
+          if (isDummyArgument(def) ||
+              def.template getDefiningOp<fir::AddrOfOp>()) {
+            v = def;
+            defOp = v.getDefiningOp();
+            return;
           }
           // No further tracking for addresses loaded from memory for now.
           type = SourceKind::Indirect;
@@ -314,24 +305,7 @@ AliasAnalysis::Source AliasAnalysis::getSource(mlir::Value v) {
         .Case<fir::AddrOfOp>([&](auto op) {
           // Address of a global scope object.
           ty = v.getType();
-
-          // When the global is a
-          // fir.global @_QMpointersEp : !fir.box<!fir.ptr<f32>>
-          //   or
-          // fir.global @_QMpointersEp : !fir.box<!fir.heap<f32>>
-          //
-          // and when following through the wrapped address, capture
-          // the fact that there is nothing known about it. Therefore setting
-          // the source to Direct.
-          //
-          // When not following the wrapped address, then consider the address
-          // of the box, which has nothing to do with the wrapped address and
-          // lies in the global memory space.
-          if (followBoxAddr &&
-              mlir::isa<fir::BaseBoxType>(fir::unwrapRefType(ty)))
-            type = SourceKind::Direct;
-          else
-            type = SourceKind::Global;
+          type = SourceKind::Global;
 
           auto globalOpName = mlir::OperationName(
               fir::GlobalOp::getOperationName(), defOp->getContext());
@@ -339,11 +313,10 @@ AliasAnalysis::Source AliasAnalysis::getSource(mlir::Value v) {
                   v, fir::GlobalOp::getTargetAttrName(globalOpName)))
             attributes.set(Attribute::Target);
 
-          // TODO: Take followBoxAddr into account when setting the pointer
+          // TODO: Take followBoxData into account when setting the pointer
           // attribute
           if (Source::isPointerReference(ty))
             attributes.set(Attribute::Pointer);
-
           global = llvm::cast<fir::AddrOfOp>(op).getSymbol();
           breakFromLoop = true;
         })
@@ -389,7 +362,7 @@ AliasAnalysis::Source AliasAnalysis::getSource(mlir::Value v) {
           // MustAlias after going through a designate operation
           approximateSource = true;
           if (mlir::isa<fir::BaseBoxType>(v.getType()))
-            followBoxAddr = true;
+            followBoxData = true;
         })
         .Default([&](auto op) {
           defOp = nullptr;
@@ -408,10 +381,10 @@ AliasAnalysis::Source AliasAnalysis::getSource(mlir::Value v) {
         attributes.set(Attribute::Pointer);
     }
 
-  if (type == SourceKind::Global || type == SourceKind::Direct)
-    return {global, type, ty, attributes, approximateSource};
-
-  return {v, type, ty, attributes, approximateSource};
+  if (type == SourceKind::Global) {
+    return {{global, followingData}, type, ty, attributes, approximateSource};
+  }
+  return {{v, followingData}, type, ty, attributes, approximateSource};
 }
 
 } // namespace fir
diff --git a/flang/lib/Optimizer/Transforms/AddAliasTags.cpp b/flang/lib/Optimizer/Transforms/AddAliasTags.cpp
index 684aa4462915e5..3884c370b877b9 100644
--- a/flang/lib/Optimizer/Transforms/AddAliasTags.cpp
+++ b/flang/lib/Optimizer/Transforms/AddAliasTags.cpp
@@ -144,7 +144,7 @@ void AddAliasTagsPass::runOnAliasInterface(fir::FirAliasTagOpInterface op,
       source.kind == fir::AliasAnalysis::SourceKind::Argument) {
     LLVM_DEBUG(llvm::dbgs().indent(2)
                << "Found reference to dummy argument at " << *op << "\n");
-    std::string name = getFuncArgName(source.u.get<mlir::Value>());
+    std::string name = getFuncArgName(source.origin.u.get<mlir::Value>());
     if (!name.empty())
       tag = state.getFuncTree(func).dummyArgDataTree.getTag(name);
     else
@@ -154,8 +154,9 @@ void AddAliasTagsPass::runOnAliasInterface(fir::FirAliasTagOpInterface op,
 
     // TBAA for global variables
   } else if (enableGlobals &&
-             source.kind == fir::AliasAnalysis::SourceKind::Global) {
-    mlir::SymbolRefAttr glbl = source.u.get<mlir::SymbolRefAttr>();
+             source.kind == fir::AliasAnalysis::SourceKind::Global &&
+             !source.isBoxData()) {
+    mlir::SymbolRefAttr glbl = source.origin.u.get<mlir::SymbolRefAttr>();
     const char *name = glbl.getRootReference().data();
     LLVM_DEBUG(llvm::dbgs().indent(2) << "Found reference to global " << name
                                       << " at " << *op << "\n");
@@ -163,9 +164,10 @@ void AddAliasTagsPass::runOnAliasInterface(fir::FirAliasTagOpInterface op,
 
     // TBAA for SourceKind::Direct
   } else if (enableDirect &&
-             source.kind == fir::AliasAnalysis::SourceKind::Direct) {
-    if (source.u.is<mlir::SymbolRefAttr>()) {
-      mlir::SymbolRefAttr glbl = source.u.get<mlir::SymbolRefAttr>();
+             source.kind == fir::AliasAnalysis::SourceKind::Global &&
+             source.isBoxData()) {
+    if (source.origin.u.is<mlir::SymbolRefAttr>()) {
+      mlir::SymbolRefAttr glbl = source.origin.u.get<mlir::SymbolRefAttr>();
       const char *name = glbl.getRootReference().data();
       LLVM_DEBUG(llvm::dbgs().indent(2) << "Found reference to direct " << name
                                         << " at " << *op << "\n");
@@ -181,7 +183,8 @@ void AddAliasTagsPass::runOnAliasInterface(fir::FirAliasTagOpInterface op,
   } else if (enableLocalAllocs &&
              source.kind == fir::AliasAnalysis::SourceKind::Allocate) {
     std::optional<llvm::StringRef> name;
-    mlir::Operation *sourceOp = source.u.get<mlir::Value>().getDefiningOp();
+    mlir::Operation *sourceOp =
+        source.origin.u.get<mlir::Value>().getDefiningOp();
     if (auto alloc = mlir::dyn_cast_or_null<fir::AllocaOp>(sourceOp))
       name = alloc.getUniqName();
     else if (auto alloc = mlir::dyn_cast_or_null<fir::AllocMemOp>(sourceOp))
diff --git a/flang/test/Analysis/AliasAnalysis/alias-analysis-2.fir b/flang/test/Analysis/AliasAnalysis/alias-analysis-2.fir
index 31459ef21d947c..af3e491e062c3e 100644
--- a/flang/test/Analysis/AliasAnalysis/alias-analysis-2.fir
+++ b/flang/test/Analysis/AliasAnalysis/alias-analysis-2.fir
@@ -37,14 +37,14 @@
 
 // arg2 is a reference to a pointer. Modifying arg2 could
 // modify a target with a pointer component
-// CHECK-DAG: func.region0#0 <-> func.region0#2: MayAlias
-// CHECK-DAG: func.region0#1 <-> func.region0#2: MayAlias
+// CHECK-DAG: arg2.load#0 <-> func.region0#0: MayAlias
+// CHECK-DAG: arg2.load#0 <-> func.region0#1: MayAlias
 
 // However, the address wrapped by arg2, can alias with any target or
 // pointer arguments
 // CHECK-DAG: arg2.addr#0 <-> func....
[truncated]

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.

Thanks Renaud, please wait on further approval WRT the comments from other reviewers, but the code logic looks great to me.

flang/include/flang/Optimizer/Analysis/AliasAnalysis.h Outdated Show resolved Hide resolved
flang/include/flang/Optimizer/Analysis/AliasAnalysis.h Outdated Show resolved Hide resolved
flang/include/flang/Optimizer/Analysis/AliasAnalysis.h Outdated Show resolved Hide resolved
flang/include/flang/Optimizer/Analysis/AliasAnalysis.h Outdated Show resolved Hide resolved
flang/include/flang/Optimizer/Analysis/AliasAnalysis.h Outdated Show resolved Hide resolved
flang/test/Analysis/AliasAnalysis/alias-analysis-2.fir Outdated Show resolved Hide resolved
flang/test/Analysis/AliasAnalysis/alias-analysis-3.fir Outdated Show resolved Hide resolved
flang/lib/Optimizer/Analysis/AliasAnalysis.cpp Outdated Show resolved Hide resolved
flang/test/Analysis/AliasAnalysis/alias-analysis-9.fir Outdated Show resolved Hide resolved
Comment on lines 26 to 36
// CHECK-DAG: x#0 <-> xnext1#0: MayAlias
// CHECK-DAG: x#0 <-> xnext2#0: MayAlias
// CHECK-DAG: xnext1#0 <-> xnext2#0: MayAlias
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you add a brief explanation of why these alias? I get it from Jean's earlier example, but it seems subtle enough to be worthwhile to document here.

Also, wouldn't MustAlias be better in all three cases, and can we document that here? That is, for the duration of a call to foo, the address of x or of its next pointer cannot change, right? Thus, xnext1 and xnext2 must be the same (even if the loads from them might produce different addresses).

Copy link
Contributor Author

@Renaud-K Renaud-K May 13, 2024

Choose a reason for hiding this comment

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

We save MustAlias when the starting points match exactly. If we reach the source through an offset calculation we set approximateSource and return MayAlias. c3f060b

As far, xnext1#0 <-> xnext2#0, we could do better and return MustAlias but as @tblah points out in his commit, we do not compare operands used in offset calculations.

Copy link
Contributor

@tblah tblah left a comment

Choose a reason for hiding this comment

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

LGTM once other reviewers are happy and CI passes. I don't see any performance regressions from a very non-exhaustive check.

Thank you for your work on this. I think it is a lot easier to understand than the older approach.

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.

Thanks for the updates. Just a few more minor points.

Comment on lines 66 to 68
// a(1) = 1
// -------------------------------------------------
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// a(1) = 1
// -------------------------------------------------
// a(1) = 1
// end subroutine
// -------------------------------------------------

@@ -108,12 +108,11 @@ AliasResult AliasAnalysis::alias(Value lhs, Value rhs) {
auto lhsSrc = getSource(lhs);
auto rhsSrc = getSource(rhs);
bool approximateSource = lhsSrc.approximateSource || rhsSrc.approximateSource;
LLVM_DEBUG(llvm::dbgs() << "AliasAnalysis::alias\n";
LLVM_DEBUG(llvm::dbgs() << "\n"; llvm::dbgs() << "AliasAnalysis::alias\n";
Copy link
Collaborator

Choose a reason for hiding this comment

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

This issue remains.

Comment on lines 27 to 28
// Pointer components may alias with pointer objects,
// as well as other pointer components
Copy link
Collaborator

Choose a reason for hiding this comment

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

For this comment to be relevant, x#0 must be considered either a pointer component or pointer object. But x#0 is like y#0, which the previous block says is not a pointer object and does not alias xnext1#0 or xnext2#0. Please clarify.

Copy link
Contributor Author

@Renaud-K Renaud-K May 15, 2024

Choose a reason for hiding this comment

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

git-clang-format is moving the llvm::dbgs statement next to the first one. But I can do this:
llvm::dbgs() << "\nAliasAnalysis::alias\n";

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 once CI passes. Thanks!

@Renaud-K
Copy link
Contributor Author

I tried to commit to my fork a rebase of the branch. Sorry if it caused confusion.

@Renaud-K Renaud-K force-pushed the rk-non-data-aliasing branch from 6d18387 to 804a4b9 Compare May 15, 2024 22:40
@Renaud-K Renaud-K merged commit ee407e1 into llvm:main May 16, 2024
4 checks passed
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.

6 participants