This is an archive of the discontinued LLVM Phabricator instance.

[LICM] Hoist calls to readonly argmemonly functions even with stores in the loop
ClosedPublic

Authored by reames on Sep 10 2015, 1:11 PM.

Details

Summary

We know that an argmemonly function can only access memory pointed to by it's arguments. Rather than needing to consider all possible stores as aliasing (as we do for a readonly function), we can only consider the aliasing of the pointer arguments.

Note that this change only addresses hoisting. I'm thinking about how to address speculation safety as well, but that will be a different change.

Diff Detail

Event Timeline

reames updated this revision to Diff 34481.Sep 10 2015, 1:11 PM
reames retitled this revision from to [LICM] Hoist calls to readonly argmemonly functions even with stores in the loop.
reames updated this object.
reames added reviewers: hfinkel, igor-laevsky, sanjoy.
reames added a subscriber: llvm-commits.

Can't a readonly, argmemonly function trap?

Can't a readonly, argmemonly function trap?

Absolutely. But that's not an aliasing property. That's handled by isGuaranteedToExecute.

sanjoy accepted this revision.Sep 15 2015, 9:40 PM
sanjoy edited edge metadata.

The code looks pretty straightforward, so I'm okay giving an LGTM; but given that I'm not specifically familiar with LICM, it'd be understandable if you want to wait for some more feedback.

This revision is now accepted and ready to land.Sep 15 2015, 9:40 PM
reames added a subscriber: reames.Sep 16 2015, 7:34 PM

Hal, David,

Could one of you take a quick look?

Philip

majnemer added inline comments.Sep 16 2015, 7:53 PM
lib/Transforms/Scalar/LICM.cpp
468–471

Is the function permitted to bitcast it's non-pointer parameters and store to them?

majnemer accepted this revision.Sep 16 2015, 7:55 PM
majnemer added a reviewer: majnemer.

LGTM

lib/Transforms/Scalar/LICM.cpp
468–471

Ah, the langref answers this question...

sanjoy added inline comments.Sep 16 2015, 7:56 PM
lib/Transforms/Scalar/LICM.cpp
468–471

I had the exact same question earlier. :) Turns out, the specification for argmemonly forbids that -- an argmemonly function can only store through *pointer typed* arguments.

"This attribute indicates that the only memory accesses inside function are loads and stores from objects pointed to by its pointer-typed arguments, with arbitrary offsets."

This revision was automatically updated to reflect the committed changes.