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

[Joel E. Denny via flang-commits]

jdenny-ornl wrote:

> Thanks @jdenny-ornl for the update. This looks better!

Thanks for reviewing.

> I am still a bit bothered by the fact that the code is turned in a way were it is proving aliasing instead of no aliasing (i.e., it is doing `if (cdt1) then mayalias else if (cdt2) then mayalias .... else noalias` ), this makes it harder to validate that aliasing cases are not missing via human review (at least in my experience). But I have to concede the standard is a bit written in that way in section 15.5.2.14 of F2023, and I do not have immediate suggestions to reverse the logic in a clearer way.

I think that is a description of [fir::AliasAnalysis::alias](https://github.com/llvm/llvm-project/blob/3b703d479ff37883242acc20fed317ed8a5466dc/flang/lib/Optimizer/Analysis/AliasAnalysis.cpp#L97-L181), and it holds with or without this PR.  I agree the reverse seems safer, but I don't know how to achieve it either.

> Can I ask you to push the `isRecordWithPointerComponent` and `isPointerReference` NFC change as well as the alias-analysis-9.fir renaming in a preliminary patch so that your functional changes stands out?

Done in PR #105899.  I haven't bothered with a stacked PR as I assume that will land quickly, but let me know if that would be helpful.

> Then I will run this update through some end-to-end apps and keep you posted.

Sounds great.  Thanks.

> Do you know any apps where I should see performance improvement

No, not so far.

> (i.e., was your change motivated by some use case in some app?).

Yes, indirectly.  I have developed some Flang alias analysis improvements that benefit the performance of LLNL's UMT proxy app. However, those improvements alter some alias analysis results in Flang's test suite.  Often where the existing expected result is MayAlias, I was originally not sure whether authors felt that is a necessary result or just a conservative result due to limitations of the current implementation.  I also then doubted new tests I was adding.  I wrote the RFC to get answers so I could make further progress on my improvements.  Developing this PR seemed like the best way to make the RFC discussion concrete.

> Note that in general I think we should try to change aliasing code in the future either to simplify the logic, or when motivated by some actual performance use case because these changes need thorough review, which require a lot of brain bandwidth. I have limited faith in end-to-end testing currently for aliasing given flang has a rather limited usage of it, so my concern is the introduction of hard to catch dormant optimization bugs (so thank you for all the LIT tests you are adding in your patch, this is great).

I understand.  When I started the RFC, I wasn't sure whether we'd end up landing changes to the implementation or just the test suite comments.  I hope the PR we ended up with serves to clarify both even as it exposes more detailed use cases in the Fortran standard.

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