This is an archive of the discontinued LLVM Phabricator instance.

Allow value forwarding past release fences in GVN
ClosedPublic

Authored by reames on Jul 22 2015, 3:57 PM.

Details

Summary

This is the GVN companion to http://reviews.llvm.org/D11434. See the description there for a higher level discussion of release fence semantics.

I *think* this patch is correct, but I would definitely appreciate really careful review. Because MDA is used by both GVN and DeadStoreElimination, I had to restrict the dependency bypass to only apply for loads. This feels like an utter hack, but we don't seem to have a meaningful return value for "reads but does not clobber". As a result, I need to return a clobber result for DSE, but skip this instruction for GVN. We have the same pattern in other places, but ... yuck.

Diff Detail

Event Timeline

reames updated this revision to Diff 30414.Jul 22 2015, 3:57 PM
reames retitled this revision from to Allow value forwarding past release fences in GVN.
reames updated this object.
jfb added inline comments.Jul 24 2015, 9:26 AM
test/Transforms/DeadStoreElimination/fence.ll
13

Same as in D11434: the second store can be eliminated because this is a race. You still need to store the second value, so it would be nice to have 5 and something else, and a comment that explains that you can but choose not to.

test/Transforms/GVN/fence.ll
43

It would be good to also explain that this is racy, but we choose to not exercise our optimization right. Putting a loop and having proper code here would defeat the optimizer, whereas this test is the most basic check that the optimizer is being nice.

hfinkel added inline comments.Jul 25 2015, 12:20 PM
test/Transforms/GVN/fence.ll
43

While I understand the value of small test cases, is it not simple to construct a non-racy test case?

reames planned changes to this revision.Aug 26 2015, 6:37 PM

Given the associated EarlyCSE patch just landed, I plan to update this revision in the next few days. The source code won't change, but I plan to revise the tests slightly to try to avoid obvious races. Given I don't claim to be an expert in the intricacies of the C++ memory model, we'll see how successful I am.

reames updated this revision to Diff 42206.Dec 8 2015, 11:43 AM

Revive an old review thread. The associated EarlyCSE change has landed and hasn't appeared to turn up any problems. I've tried to revise the test cases to be clear about what's going on and why.

@JF - Can I get a review for this one? It came up again on a benchmark I'm looking at and I'd really like to get it in.

jfb edited edge metadata.Mar 25 2016, 2:25 PM

Two comments and then this is good. I'm much more comfortable testing that we're doing exactly what we think we are!

lib/Analysis/MemoryDependenceAnalysis.cpp
705

Typo: "isn't".

test/Transforms/DeadStoreElimination/fence.ll
13

I think this still applies. I'd like another test which does:

; CHECK: store i32 42
; CHECK: fence release
; CHECK: store i32 1
; CHECK: ret
  store i32 42, i32* %addr.i, align 4
  fence release
  store i32 1, i32* %addr.i, align 4
  ret void

I want the test to ensure that it's not just about the store value being the same at the same address, but also that the values being equal doesn't matter. If we decide to optimize further then the result would be:

; CHECK: store i32 1
; CHECK: fence release
; CHECK: ret

Once comments are addressed (~20 minutes from now, waiting for build), do you want another round of review?

lib/Analysis/MemoryDependenceAnalysis.cpp
705

inst is instruction. I'll rephrase as:
"if the query instruction is a store."

test/Transforms/DeadStoreElimination/fence.ll
13

Will add this test and comment appropriately.

jfb accepted this revision.Mar 25 2016, 2:46 PM
jfb edited edge metadata.

Once comments are addressed (~20 minutes from now, waiting for build), do you want another round of review?

No that's good.

lib/Analysis/MemoryDependenceAnalysis.cpp
705

Oops yes!

This revision is now accepted and ready to land.Mar 25 2016, 2:46 PM
This revision was automatically updated to reflect the committed changes.