Page MenuHomePhabricator

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

Authored by yamauchi 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.

Diff Detail

Event Timeline

yamauchi 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.

Any comment?

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
487

can you guard the change with

if (!AA) {
}

as we AA, it will be unnecessary.

yamauchi updated this revision to Diff 238831.Jan 17 2020, 10:54 AM
yamauchi 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)

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)

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.Mon, Jan 27, 4:08 PM

Thanks

xbolva00 added inline comments.Mon, Jan 27, 4:20 PM
llvm/lib/Analysis/Loads.cpp
483–484

else {

?

yamauchi marked an inline comment as done.Tue, Jan 28, 8:31 AM
yamauchi added inline comments.
llvm/lib/Analysis/Loads.cpp
483–484

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.Tue, Jan 28, 9:35 AM
llvm/lib/Analysis/Loads.cpp
483–484

Yes

(optional, if you like, do so)

yamauchi updated this revision to Diff 241261.Wed, Jan 29, 12:49 PM
yamauchi marked an inline comment as done.

Address comment (combine the if statements).

This revision was automatically updated to reflect the committed changes.