[flang-commits] [flang] [flang] AliasAnalysis: distinguish addr of arg vs. addr in arg (PR #87723)

[Joel E. Denny via flang-commits]

================
@@ -399,19 +404,30 @@ AliasAnalysis::Source AliasAnalysis::getSource(mlir::Value v) {
   if (!defOp && type == SourceKind::Unknown)
     // Check if the memory source is coming through a dummy argument.
     if (isDummyArgument(v)) {
-      type = SourceKind::Argument;
       ty = v.getType();
       if (fir::valueHasFirAttribute(v, fir::getTargetAttrName()))
         attributes.set(Attribute::Target);
-
       if (Source::isPointerReference(ty))
         attributes.set(Attribute::Pointer);
+      if (followBoxAddr && fir::isa_ref_type(ty))
----------------
jdenny-ornl wrote:

> With your change, we are going to have to revisit how we detect a dummy argument. The SourceKind no longer captures it. Long story short, I think it is fine. We may want to also revisit the SourceKind nomenclature at some point but I think that's a different issue. When the search up the use-def chain reaches the same mlir::Value for a box and its data, we need some way of encoding that they are different and I guess the SourceKind is what we have used for that.

As far as I know, this patch only affects the case of an address extracted from a fortran pointer that is a dummy argument.  My assumption was that it's fine to just label that extracted address with `SourceKind::Direct`.  For what use case do we need to know that it's from a dummy argument?

> The host association test still needs to know that it's dealing with a dummy

That test doesn't deal with a fortran pointer, so this patch leaves it as `SourceKind::Argument`.

> and so instead of checking for SourceKind we can check for the mlir::Value. The check would become something like
> 
> ```
> if ((src1->isDummyArgument() && src2->kind == SourceKind::HostAssoc) || (src2->isDummyArgument() && src1->kind)
> ```
> 
> The `fir::isa_ref_type(ty)` restriction seems arbitrary.

Recall that, a few comments ago, I offered an alternative implementation that more clearly checks for the pointer case that this patch is trying to address.  Maybe I should update the patch to that implementation now to facilitate further discussion?

> I understand that it helps with this test and limits updates of the tbaa tests but ultimately I would think you would want to apply it unrestricted to rip all the benefits.

What additional benefits do you mean?

> I will leave it up to you and Tom to decide which way to go since you are going to be more impacted. And maybe @vzakhari can weigh in since he runs benchmarks on our side.
> 
> For context, @vzakhari is also going to work on a change related to dummy arguments so tbaa can be properly applied after inlining. There, too, we will need more information about a dummy and SourceKind alone will be insufficient. So, generally speaking things are moving away from the SourceKing::Argument being really useful.

Is there a reference somewhere on this work?  I'd like to hear more. I have more alias analysis patches coming, and I'd like to be aware of any potential conflicts with other work.

> I am sorry, if I made any of this tedious. I look at this code every 6 month maybe and need a refresh each time.

No, thanks for the discussion!

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