bondhugula
commented
5 years ago We should be able to expose such fix independently through tests: can one of the test passes be improved to expose this?
None of the current users of this method are actually affected by this fix, but there is another PR that I am about to submit on scalar replacement which relies on this (i.e., replaceAllMemRefUsesWith can fail and the failure is handled), and have a test case for that. However that PR is too big in itself and separate from this.
The reason the existing users always make sure that this methods succeeds is that replaceAllMemRefUsesWith is typically called by a pass/utility after a bunch of IR actions have already been performed (new op creation, etc.). So, if it were to fail, one would have to undo/clear up all of those actions. As a result, those clients always first check from the outside a-priori whether it'd succeed (by checking for non-dereferencing uses) and then call it.
antiagainst
Refactor replaceAllMemRefUsesWith to split it into two methods: the new method does the replacement on a single op, and is used by the existing one.
make the methods return LogicalResult instead of bool
Earlier, when replacement failed (due to non-deferencing uses of the memref), the set of ops that had already been processed would have been replaced leaving the IR in an inconsistent state. Now, a pass is made over all ops to first check for non-deferencing uses, and then replacement is performed. No test cases were affected because all clients of this method were first checking for non-deferencing uses before calling this method (for other reasons). This isn't true for a use case in another upcoming PR (scalar replacement); clients can now bail out with consistent IR on failure of replaceAllMemRefUsesWith.
multiple deferencing uses of the same memref in a single op is possible (we have no such use cases/scenarios), and this has always remained unsupported. Add an assertion for this.
Signed-off-by: Uday Bondhugula uday@polymagelabs.com