Page MenuHomePhabricator

[RFC][GVN] Remove redundant load by GVN
Needs ReviewPublic

Authored by shiva0217 on Mar 25 2018, 11:05 PM.
This revision needs review, but all specified reviewers are disabled or inactive.

Details

Reviewers
dberlin
Summary

Remove redundant load LI by GVN in following case:

lw      a0, Addr1 <- LItoSI
sw      a0, Addr2 <- SI
lw      a0, Addr1 <- LI

If LI and LItoSI load from the same address, the value load from LItoSI could propagate to LI even if Addr1 alias to Addr2.
Therefore, LI could eliminate by GVN in this case.

Diff Detail

Repository
rL LLVM

Event Timeline

shiva0217 created this revision.Mar 25 2018, 11:05 PM

This isn't correct in the face of atomics and volatiles, and IMHO, MemoryDependence should not be performing this optimization.
If you want, teach GVN to requery MemDep in these cases.

dberlin requested changes to this revision.Mar 26 2018, 8:51 AM
This revision now requires changes to proceed.Mar 26 2018, 8:51 AM

This isn't correct in the face of atomics and volatiles, and IMHO, MemoryDependence should not be performing this optimization.
If you want, teach GVN to requery MemDep in these cases.

Hi @dberlin.
The patch code will not apply when face atomics and volatiles due to MemoryDependence has atomic and volatile checking before entering this part.
It detects the case in the summary, and then MemDep will not clobber "LItoSI" load value by store and then GVN could remove "QueryInst" load by requiring MemDep.
So the patch will not perform optimization by itself, it will let GVN deal with it.

shiva0217 edited the summary of this revision. (Show Details)Mar 27 2018, 11:17 PM

Sorry, again, i don't think this is a thing memdep should be doing.
If GVN can't handle this sanely, that's on GVN.
So i'm not going t approve this.

shiva0217 updated this revision to Diff 140195.Mar 29 2018, 2:27 AM
shiva0217 retitled this revision from [RFC][Memory Dependency Analysis] Not clobber load value by store if the store value won't change load content to [RFC][GVN] Remove redundant load by GVN.
shiva0217 edited the summary of this revision. (Show Details)

Hi @dberlin. Thanks for your comments. I update the patch as your suggestion.
Currently, I implement in GVN.cpp: GVN::AnalyzeLoadAvailability.
Is that the place you expect to implement?

lebedev.ri added inline comments.
lib/Transforms/Scalar/GVN.cpp
894

Missing test coverage

shiva0217 updated this revision to Diff 140379.Mar 29 2018, 6:42 PM

Update the test case to address @lebedev.ri 's comments.

Hi @dberlin. Sorry, I didn't get your point in the first place. I moved the implementation from MemDep to GVN. Could you help me to check that is there anything missing? Thanks again to point me to the right direction.