Differential D32006
Mark invariant.group.barrier as inaccessiblememonly Authored by Prazek on Apr 13 2017, 4:38 AM.
Details 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
Diff Detail
Event TimelineComment Actions 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)? Comment Actions 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. Comment Actions 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 Comment Actions 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?
Comment Actions 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. Comment Actions 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? Comment Actions If that is the model then barrier can't be both writeonly *and* argmemonly, since the side table is disjoint from the object. Comment Actions I am not sure about this because the barrier is inserted before changing the dynamic type, not after. 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 I can give a brief summary of my thinking about what attributes we can have on the barrier and what can't 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): %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 + Can't be writeonly alone, because if the barrier argument would escape before it, + 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 %ptr2 = call i8* @llvm.invariant.group.barrier(i8* %ptr) %ptr3 = call i8* @llvm.invariant.group.barrier(i8* %ptr) I am not sure if I follow, because I changed argmemonly to inaccessiblememonly Comment Actions Rebase. This patch will not be commited, but will be used as a base for new development. Comment Actions 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. Comment Actions As I don't see if writeonly could help us in any way, and I am worried that in case: %a = 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. Comment Actions I agree (and, in fact, @homerdin is working on that optimization, so this likely won't be theoretical for very long). | ||||||||||
You'd need to update the comment.