[flang-commits] [flang] WIP: [flang] AliasAnalysis: Fix pointer component logic (PR #94242)

[Joel E. Denny via flang-commits]

================
@@ -122,13 +122,32 @@ AliasResult AliasAnalysis::alias(Value lhs, Value rhs) {
   }
 
   if (lhsSrc.kind == rhsSrc.kind) {
+    // If the kinds and origins are the same, then lhs and rhs must alias unless
+    // either source is approximate.  Approximate sources are for parts of the
+    // origin, but we don't have info here on which parts and whether they
+    // overlap, so we normally return MayAlias in that case.
+    //
+    // There is an exceptional case: The origins might compare unequal because
+    // only one has !isData().  If that source is approximate and the other is
+    // not, then the former is the source for the address *of* a pointer
+    // component in a composite, and the latter is for the address of that
+    // composite.  As for the address of any composite vs. the address of one of
+    // its components, a store to one can affect a load from the other, so the
+    // result is MayAlias.
     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;
     }
+    if (lhsSrc.origin.u == rhsSrc.origin.u &&
+        ((lhsSrc.approximateSource && !lhsSrc.isData() && !rhsSrc.approximateSource) ||
+         (rhsSrc.approximateSource && !rhsSrc.isData() && !lhsSrc.approximateSource))) {
+      LLVM_DEBUG(llvm::dbgs()
+                 << "  aliasing between composite and pointer component\n");
+      return AliasResult::MayAlias;
+    }
----------------
jdenny-ornl wrote:

> My concern would be to modify getOriginalDef to return anything other than Source::Indirect.

For the record, getOriginalDef does not itself choose a SourceKind.  Moreover, after getSource's LoadOp case calls getOriginalDef, getSource currently returns any of Indirect, Argument, or Global.  That is true independently of this patch.  No modification required.

(As you have noted, there are cases like alloca that getSource's LoadOp case doesn't currently fully handle.  As a result, it currently returns Indirect sometimes where, for consistency with the current handling of dummy args and globals, it should return Allocate.)

> `%11` with the designate case is always indirect. We have no idea what was stored in `%9`, it could a global, an allocatable or a dummy: we do not know.

In that example, what was stored in %9 is the address of the pointer. In my above example without hlfir.designate, %3 is the address of the pointer.  As far as I understand, the difference between those is whether that address is at a base address (for a global, dummy arg, alloca, etc.) or at an offset from that base address.

I don't see why that difference *should* affect the SourceKind for any value that is computed from the address of the pointer.  In particular, if the source origin (i.e., %2) were a dummy arg or global, the SourceKind for %11 in my above example without hflir.designate would currently be Argument or Global, but the SourceKind for %11 in my above example with hlfir.designate would currently be Indirect.  I see no reason for that difference.  In a later patch, I would like to fix that.  (Again, I would also like to add support for the alloca/Allocate case shown in the examples.)

> There is room to improve on SourceKind::Indirect, such as using the type system to distinguish !fir.ptr from other memory references but that would be a different discussion. In the example just above, `%11` would then be indirect too since we are following the data and not interested in the box address which happens to be an SourceKind::Alloca. Today, I would guess, we would get SourceKind::Alloca with an isData flag but it would be better modeled with SourceKind::Indirect and isData. Then we could do with SourceKind::Indirect what we used to do clumsily with SourceKind::Direct.

Whatever we decide to call the SourceKind for that case, do we agree that hlfir.designate should not change the SourceKind for %11 between my previous two examples?

> But I think, we should take it one step at a time.

Agreed.  Hopefully the current patch I'm trying to develop can be a next step.

> It would impact @tblah who is handling the (global, isData) separately. These two would become (indirect, isData)

Where is the discussion of his work?



https://github.com/llvm/llvm-project/pull/94242