This is an archive of the discontinued LLVM Phabricator instance.

[Loads] Handle simple cases with same base pointer with constant offsets in FindAvailableLoadedValue when AA is null.
ClosedPublic

Authored by hjyamauchi on Dec 10 2019, 3:24 PM.

Details

Summary

This will help with devirtualization (store forwarding with vtable pointers in
the presence of other stores into members in the constructor.) During inlining,
we don't have AA.

Event Timeline

hjyamauchi created this revision.Dec 10 2019, 3:24 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 10 2019, 3:24 PM

This is split off of D69591.

BasicAA is stateless and should be available and used here to disambiguate.

I believe if I request AA in Inliner.cpp and pass it down to FindAvailableLoadedValue through tryPromoteCall (D71308), it would work without this patch.

Is it okay to assume that a) the AA is BasicAA and b) BasicAA is stateless since the AA interface is more generic? Is there a way to condition this logic on those conditions?

davidxl added inline comments.Jan 17 2020, 9:28 AM
llvm/lib/Analysis/Loads.cpp
457

can you guard the change with

if (!AA) {
}

as we AA, it will be unnecessary.

hjyamauchi marked an inline comment as done.

Address comment. PTAL.

This revision is now accepted and ready to land.Jan 17 2020, 11:08 AM

(just passing-by remark)

In D71307#1792951, @yamauchi wrote:

I believe if I request AA in Inliner.cpp and pass it down to FindAvailableLoadedValue through tryPromoteCall (D71308), it would work without this patch.

Is it okay to assume that a) the AA is BasicAA and b) BasicAA is stateless since the AA interface is more generic? Is there a way to condition this logic on those conditions?

Perhaps it may be more productive to explain why this This is a simple form of alias analysis. *has* to exist?
Why can't, as suggested, BasicAA be used? What goes wrong if BasicAA is used?

To repeat/expand my question above,..

As I understand, it'd be possible to get the AA from the pass manager from the inline pass, but the AA interface is abstract and the inline pass can't assume what AA implementations they are (which is up to the pass manager setup) and they are stateless BasicAA (otherwise we'd need to invalidate/update stateful AA which is hard to do during (complex) inlining transformations and there's no guarantee that BasicAA will stay stateless in the future.) Is there a way to specifically request BasicAA and only it and that it must be stateless? The current proposed approach is to have this "simple form of alias analysis" to handle enough of the aliasing around a C++ constructor call for this missed inlining case.

(just passing-by remark)

In D71307#1792951, @yamauchi wrote:

I believe if I request AA in Inliner.cpp and pass it down to FindAvailableLoadedValue through tryPromoteCall (D71308), it would work without this patch.

Is it okay to assume that a) the AA is BasicAA and b) BasicAA is stateless since the AA interface is more generic? Is there a way to condition this logic on those conditions?

Perhaps it may be more productive to explain why this This is a simple form of alias analysis. *has* to exist?
Why can't, as suggested, BasicAA be used? What goes wrong if BasicAA is used?

I agree, it is quite bad to reinvent simple alias analysis here. If this is really a way to go, maybe move this code to own function?

As other commented, please extract the code into its own function also add the support when AA is available ( as other parts of the function does).

xbolva00 accepted this revision.Jan 27 2020, 4:08 PM

Thanks

xbolva00 added inline comments.Jan 27 2020, 4:20 PM
llvm/lib/Analysis/Loads.cpp
474

else {

?

hjyamauchi marked an inline comment as done.Jan 28 2020, 8:31 AM
hjyamauchi added inline comments.
llvm/lib/Analysis/Loads.cpp
474

Do you mean it'd be good to combine the two if statements?

if (!AA) {
  // ...
  if (AreNonOverlapSameBaseLoadAndStore(..))
    continue;
} else {
  // ...
  if (!isModSet(AA->getModRefInfo(SI, StrippedPtr, AccessSize)))
    continue;
}
xbolva00 added inline comments.Jan 28 2020, 9:35 AM
llvm/lib/Analysis/Loads.cpp
474

Yes

(optional, if you like, do so)

hjyamauchi marked an inline comment as done.

Address comment (combine the if statements).

This revision was automatically updated to reflect the committed changes.