Page MenuHomePhabricator

Remove readnone from

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



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,
which would be merged into one call, causing devirtualization
to devirtualize second call into Derived1::foo() instead of

Diff Detail


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
2 ↗(On Diff #93623)

is there more general directory?

chandlerc added inline comments.Mar 31 2017, 6:50 AM
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
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?

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


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.