This is an archive of the discontinued LLVM Phabricator instance.

[DeadStoreElimination] Check function modref behavior before considering memory clobbered
ClosedPublic

Authored by igor-laevsky on Feb 15 2017, 9:58 AM.

Details

Summary

Currently we assume that if function may write to memory and it has MRI_ModRef behaviour in relation to the requested memory location then it means that this function writes to the memory location. In fact it's possible that it only reads from it. For example we can achieve this by using readonly attribute on the function operand.

Diff Detail

Event Timeline

igor-laevsky created this revision.Feb 15 2017, 9:58 AM

So
A. This added test still fails with this patch for me :)
B. I see this is only used in the case where the code is trying to eliminate redundant stores, so seems okay otherwise.

@dberlin
A: Sorry, I should have mentioned that this change depends the alias analysis change: https://reviews.llvm.org/D29989
B: Yes, this function specifically checks for memory modifications, so it shouldn't be used to determine if store is dead.

sanjoy added inline comments.Feb 22 2017, 11:47 AM
test/Transforms/DeadStoreElimination/operand-bundles.ll
48

Looks like today -basicaa -dse does not elide the second store in

declare void @foo()
declare void @bar(i8*)

define void @test4(i8* %ptr) {
  store i8 0, i8* %ptr, align 4
  call void @foo() readonly
  store i8 0, i8* %ptr, align 4
  call void @bar(i8* nocapture %ptr)
  ret void
}

Will it be able to do that with your changes? If so, adding that as a test case will be nice.

reames accepted this revision.Feb 24 2017, 5:07 PM
reames added a subscriber: reames.

The actual code change looks obviously correct.

LGTM once the AA change lands.

test/Transforms/DeadStoreElimination/operand-bundles.ll
48

It doesn't look like we have the framework for this in DSE which is unfortunate. Oddly enough earlycse probably will get this example.

This revision is now accepted and ready to land.Feb 24 2017, 5:07 PM
This revision was automatically updated to reflect the committed changes.