This is an archive of the discontinued LLVM Phabricator instance.

Mark invariant.group.barrier as inaccessiblememonly
AbandonedPublic

Authored by Prazek on Apr 13 2017, 4:38 AM.

Details

Summary

It turned out that readonly argmemonly is not enough.

store 42, %p
%b = barrier(%p)
store 43, %b

the first store is dead, but because barrier was marked as
reading argument memory, it was considered alive. With
inaccessiblememonly it doesn't read the argument, but
it also can't be CSEd.

Event Timeline

Prazek created this revision.Apr 13 2017, 4:38 AM
Prazek set the repository for this revision to rL LLVM.Apr 16 2017, 1:38 PM
sanjoy requested changes to this revision.Apr 25 2017, 12:57 AM

Doesn't this conflict with the test case you had for D31531:

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

Won't it now be OK to CSE the two launders since the only memory changed between the two is visible to the module (and thus not read by an inaccessiblememonly readonly function)?

This revision now requires changes to proceed.Apr 25 2017, 12:57 AM

Doesn't this conflict with the test case you had for D31531:

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

Won't it now be OK to CSE the two launders since the only memory changed between the two is visible to the module (and thus not read by an inaccessiblememonly readonly function)?

I have to double check that. If this is the case then probably we can remove readonly from the barrier. This way we won't be able to CSE barrier together, but we will be able to DSE through the barrier.

Yes, you are right. I missed that in the tests. I will change it to have only inaccesiblememonly. It would be good to figure out a way to CSE 2 barriers if there is no write to memory between, because I don't see if it
is possible with current set of attributes. Fortunatelly it is not that important, we should not miss any important optimizations without that.

Prazek updated this revision to Diff 97081.Apr 28 2017, 4:56 AM
Prazek edited edge metadata.
  • Add nocapture

Because now barrier is considered as writing memory, I marked it's argument as nocapture. I am not sure if it is correct, since barrier returns it's pointer. Does it mean that it outlives the barrier?

Prazek updated this revision to Diff 97115.Apr 28 2017, 10:09 AM

Remove nocapture

hfinkel added inline comments.May 7 2017, 2:17 PM
include/llvm/IR/Intrinsics.td
620

You'd need to update the comment.

Prazek updated this revision to Diff 98158.May 8 2017, 5:47 AM
  • Fix comment
Prazek marked an inline comment as done.May 8 2017, 5:49 AM

Is it possible to add "writeonly"? I am not sure if it will help in any way, but the tests seems to be working it.

hfinkel edited edge metadata.May 8 2017, 8:13 AM

Is it possible to add "writeonly"? I am not sure if it will help in any way, but the tests seems to be working it.

Yes, I think this makes sense. The model is that there's some side table holding the object type of all memory, and this barrier represents places where we might be updating that table to assign a different type. Right?

sanjoy added a comment.May 8 2017, 8:28 AM

Is it possible to add "writeonly"? I am not sure if it will help in any way, but the tests seems to be working it.

Yes, I think this makes sense. The model is that there's some side table holding the object type of all memory, and this barrier represents places where we might be updating that table to assign a different type. Right?

If that is the model then barrier can't be both writeonly *and* argmemonly, since the side table is disjoint from the object.

Prazek added a comment.May 8 2017, 3:27 PM

Is it possible to add "writeonly"? I am not sure if it will help in any way, but the tests seems to be working it.

Yes, I think this makes sense. The model is that there's some side table holding the object type of all memory, and this barrier represents places where we might be updating that table to assign a different type. Right?

I am not sure about this because the barrier is inserted before changing the dynamic type, not after.
Even the Chandler's model was invalid in that way (the model was that the barrier returns pointer that aliases the argument, but
it has some special bits that represents the dyanmic type).

Maybe we can think about this as returning pointer aliasing the argument, but with some bits set in "unique" way, so that 2 barriers calls on the same pointer
returns different value?

I can give a brief summary of my thinking about what attributes we can have on the barrier and what can't
Argument:
+ readonly (alone): we can't read the argument because we won't be able to perform DSE

store 42, %p ; dead store
%b = barrier(%p)
store 43, %b

+ writeonly (alone): we can't write through the argument because we won't be able to get values through the barrier:

store 42, %p
%b = barrier(%p)
load %b

+ the only possible way is to have it as readnone (equivalent or stronger)

Function attributes (considering that the argument is readnone):
+ Can't be readonly alone, because it will be possible to remove barrier like:

%b1 = barrier(%p)
store 42, %b1, !invariant.group
%b2 = barrier(%p) ; We could replace it with %b1 because  
; %p didn't escape, and %p aliases with %b1, so
; the barrier would read the same memory as %b1

I haven't checked that with the Capture tracking patch https://reviews.llvm.org/D32673
but if my thinking is correct then one day it could brake

+ Can't be writeonly alone, because if the barrier argument would escape before it,
we would consider barrier changing the value.

+ Can't be inaccessiblememonly & readonly, because we would be able to CSE 2 barriers like:

%ptr2 = call i8* @llvm.invariant.group.barrier(i8* nonnull %ptr)
store i8 43, i8* %ptr2, align 1, !invariant.group !0
%ptr3 = call i8* @llvm.invariant.group.barrier(i8* nonnull %ptr)

Because the store would not change inaccessible memory

+ Can be marked as inaccessiblememonly & writeonly, but
unfortunatelly we will loose the ability to CSE barriers like:

%ptr2 = call i8* @llvm.invariant.group.barrier(i8* %ptr)
%ptr3 = call i8* @llvm.invariant.group.barrier(i8* %ptr)

If that is the model then barrier can't be both writeonly *and* argmemonly, since the side table is disjoint from the object.

I am not sure if I follow, because I changed argmemonly to inaccessiblememonly

friendly ping

Prazek added a comment.Jun 6 2017, 8:42 AM

friendly ping2

friendly ping 3

Prazek updated this revision to Diff 140467.Mar 30 2018, 12:18 PM

Rebase. This patch will not be commited, but will be used as a base for new development.

kuhar accepted this revision.Mar 30 2018, 3:36 PM
kuhar added a subscriber: kuhar.

LGTM

ping Sajnoy/Richard

LGTM

Based on your document, I think it's correct for this to additionally be marked writeonly. I think the key question here is: is the side-channel information in the notional "fat pointer" established at the point where the barrier is executed, or at the first !invariant.group load through the pointer after the barrier? If it's at the point of the barrier, then the barrier notionally performs a read through its argument and shouldn't be writeonly, but your document says that the intrinsic merely establishes a new invariant group, and that only future !invariant.group loads in that group actually determine the invariant value. As well as being more optimizable, this also seems like a nicer model for frontend code generation.

Prazek added a comment.May 1 2018, 1:03 PM

As I don't see if writeonly could help us in any way, and I am worried that in case:

%a = launder(%p)
%b = launder(%p)

%a could potentially be optimized away, even if it would have load of %a in beetween (because of inaccessiblememonly), so I will leave it like this for now.

As I don't see if writeonly could help us in any way, and I am worried that in case:

%a = launder(%p)
%b = launder(%p)

%a could potentially be optimized away, even if it would have load of %a in beetween (because of inaccessiblememonly), so I will leave it like this for now.

I agree (and, in fact, @homerdin is working on that optimization, so this likely won't be theoretical for very long).

Prazek accepted this revision.May 2 2018, 1:38 AM

Thanks, pushed to master. Trying to close the revision right now.

Prazek abandoned this revision.May 2 2018, 1:50 AM

Can't close it somehow.