Mark MergedLoadStoreMotion as not preserving MemDep results #102
Conversation
MemDep caches results that signify that a dependence is non-local, and there is currently no way to invalidate such cache entries. Unfortunately, when MLSM sinks a store that can result in a non-local dependence becoming a local one, and then MemDep gives wrong answers. The easiest way out here is to just say that MLSM does indeed not preserve MemDep results.
|
Thanks! We're currently on the verge of upgrading to LLVM 5/6 though and these sorts of patches are sort of hard to carry across versions. Do we think that by the time we upgrade though LLVM will have an upstream fix to backport? |
|
According to @nagisa, this would be relatively easy to port over to LLVM 6. This is fixing a segfault, so it's fairly important to have in general. (And I don't think the problem is fixed by LLVM 6, right?) |
|
This issue is not yet fixed in LLVM 6 and should be fairly straightforward to port over. |
|
I hope that this gets fixed upstream rather sooner than later. I could also strip this patch down to just remove just the part that marks MemDep as preserved, which would be super trivial (remove 1 line, changed another). The MemDep cache invalidations are just completely useless then. |
|
Ok, let's go with it then. I'm just slightly worried b/c it's a segfault introduced by our earlier patch, but if it's easy to port it's easy to port! |
|
@alexcrichton that other patch didn't really introduce a bug, it just made us hit an existing one. As far as I'm concerned, we've just been lucky before. |
|
@dotdash ok thanks for the info! If you've got the time this sounds like an excellent candidate to upstream! |
|
@alexcrichton I'm basically waiting on feedback on the bug report I filed for this -- https://bugs.llvm.org//show_bug.cgi?id=36063 -- because I'm not sure if that's the "right" solution. Because the caching seems more deeply broken to me than just MLSM. The invalid results are only thrown away after a pass has finished, so a pass that sinks instructions like MLSM but also queries MemDep could still see bogus answers, because the cache entries become stale while the pass is still running. |
|
Ok awesome, thanks for the info! |
MemDep caches results that signify that a dependence is non-local, and
there is currently no way to invalidate such cache entries.
Unfortunately, when MLSM sinks a store that can result in a non-local
dependence becoming a local one, and then MemDep gives wrong answers.
The easiest way out here is to just say that MLSM does indeed not
preserve MemDep results.