Page MenuHomePhabricator

[AliasSetTracker] Do not treat experimental_guard intrinsic as memory writing instruction
ClosedPublic

Authored by mkazantsev on Aug 9 2018, 1:59 AM.

Details

Summary

The experimental_guard intrinsic has memory write semantics to model the thread-exiting
logic, but does not do any actual writes to memory. Currently, AliasSetTracker treats it as a
normal memory write. As result, a loop-invariant load cannot be hoisted out of loop because
the guard may possibly alias with it.

This patch makes AliasSetTracker so that it doesn't treat guards as memory writes.

Diff Detail

Repository
rL LLVM

Event Timeline

mkazantsev created this revision.Aug 9 2018, 1:59 AM
reames requested changes to this revision.Aug 9 2018, 2:22 PM
reames added inline comments.
lib/Analysis/AliasSetTracker.cpp
446 ↗(On Diff #159877)

This is incorrect. Guards don't write, but they do read everything. As such, they must participate in the aliasing with the Ref state.

counter example:
for (int i = 0; i < N; i++)

guard(LIC);
store x = 5;

}

The store and guard must in the same alias set.

This revision now requires changes to proceed.Aug 9 2018, 2:22 PM
mkazantsev planned changes to this revision.Aug 9 2018, 8:59 PM

Looking for a correct way to solve this.

mkazantsev edited the summary of this revision. (Show Details)

I hope that this approach is a correct one. Also added a dedicated test.

reames requested changes to this revision.Aug 10 2018, 9:02 AM

You need much more careful and exhaustive testing. At minimum, I want to see combinations of multiple guards, load/guard, store/guard, unrelated call/guard, etc..

test/Analysis/AliasSet/intrinsics.ll
21 ↗(On Diff #160058)

Ah, not sure what went wrong here, but this output is wrong. a, and b, must be in the same AliasSet as the guard. Both a and b modify memory read by the guard. (Unless, this might be specific to allocas which are thread local... if so, you need testing for the case where a and b are not alloca!)

25 ↗(On Diff #160058)

You can simplify this by making the argument the i1.

This revision now requires changes to proceed.Aug 10 2018, 9:02 AM
mkazantsev added inline comments.Aug 12 2018, 6:37 PM
test/Analysis/AliasSet/intrinsics.ll
21 ↗(On Diff #160058)

My best guess is that locally allocated objects never alias. I will add more tests to see what happens.

25 ↗(On Diff #160058)

I can, but I just copied it from the test above to clearly show the output difference.

Added some tests.

JFYI: I've run our internal Fuzzer suite with this patch (5k tests) and it passed ok.

reames requested changes to this revision.Aug 13 2018, 10:44 AM

Request for additional specific tests has not been addressed. See previous review comment.

lib/Analysis/AliasSetTracker.cpp
173 ↗(On Diff #160311)

nitpick, but can you change this comment to:
// guards are marked as modifying memory for control flow modelling purposes, but don't actually modify any specific memory location.

This revision now requires changes to proceed.Aug 13 2018, 10:44 AM

Will marking guards with inaccessiblememonly do the right thing?

Even if we have to make this same change to AST after using inaccessiblememonly for guards, that seems cleaner than special casing guards here.

Will marking guards with inaccessiblememonly do the right thing?

No, guard's read semantics applies to accessible memory as well.

Rebased on top of my new comprehensive tests of alias set tracker for guards, fixed comment.

mkazantsev marked an inline comment as done.Aug 14 2018, 12:31 AM
reames accepted this revision.Aug 14 2018, 12:55 PM

LGTM. Thanks for all the testing enhancements!

This revision is now accepted and ready to land.Aug 14 2018, 12:55 PM
This revision was automatically updated to reflect the committed changes.