This is an archive of the discontinued LLVM Phabricator instance.

Remove readnone from invariant.group.barrier
ClosedPublic

Authored by Prazek on Mar 31 2017, 5:57 AM.

Details

Summary

Readnone attribute would cause CSE of two barriers with
the same argument, which is invalid by example:

struct Base {
      virtual int foo() { return 42; }
};

struct Derived1 : Base {
      int foo() override { return 50; }
};

struct Derived2 : Base {
      int foo() override { return 100; }
};

void foo() {
    Base *x = new Base{};
    new (x) Derived1{};
    int a = std::launder(x)->foo();
    new (x) Derived2{};
    int b = std::launder(x)->foo();
}

Here 2 calls of std::launder will produce @llvm.invariant.group.barrier,
which would be merged into one call, causing devirtualization
to devirtualize second call into Derived1::foo() instead of
Derived2::foo()

Diff Detail

Repository
rL LLVM

Event Timeline

Prazek created this revision.Mar 31 2017, 5:57 AM
chandlerc edited edge metadata.Mar 31 2017, 6:29 AM

This looks good to me but I'd like Hal to also have a look to make sure I'm not missing anything.

Prazek added inline comments.Mar 31 2017, 6:47 AM
test/Transforms/EarlyCSE/invariant.group.barrier.ll
2 ↗(On Diff #93623)

is there more general directory?

chandlerc added inline comments.Mar 31 2017, 6:50 AM
test/Transforms/EarlyCSE/invariant.group.barrier.ll
2 ↗(On Diff #93623)

test/Other maybe?

Prazek added a subscriber: amharc.Mar 31 2017, 9:52 AM
Prazek updated this revision to Diff 93678.Mar 31 2017, 10:01 AM

moved test

amharc added inline comments.Mar 31 2017, 10:17 AM
test/Transforms/EarlyCSE/invariant.group.barrier.ll
16 ↗(On Diff #93623)

Wouldn't it be more readable to call 2x use + 1x clobber instead of 3x clobber? Only one of the clobbers necessitates a reload below, and the others are here only to check if ptr3 is replaced with ptr2, right?

test/Transforms/Util/MemorySSA/invariant-groups.ll
24 ↗(On Diff #93623)

Unrelated to this patch, but: the comment mentions MemoryUse(1), but the check below looks for MemoryUse(2).

Prazek updated this revision to Diff 93680.Mar 31 2017, 10:29 AM

Addressing Krzysztof's comments

Prazek marked an inline comment as done.Mar 31 2017, 11:14 AM
Prazek added inline comments.
test/Transforms/Util/MemorySSA/invariant-groups.ll
24 ↗(On Diff #93623)

Pushed nfc commit fixing it to trunk, but I won't rebase this patch for now, so consider this done.

Prazek marked 2 inline comments as done.Mar 31 2017, 2:06 PM
hfinkel accepted this revision.Apr 12 2017, 1:37 PM

LGTM.

Maybe we can do better, but this seems reasonable for now.

This revision is now accepted and ready to land.Apr 12 2017, 1:37 PM
This revision was automatically updated to reflect the committed changes.