llvm-project
llvm-project copied to clipboard
mlir: add getUniqueUsers
A gotcha I recently learned is that Value::getUsers() doesn't dedupe the users if there are multiple uses by one operation. There should be an easy way to get this functionality when that is what is needed. I suggest calling this Value::getUniqueUsers()
Re: https://github.com/llvm/torch-mlir/pull/1228 cc @ramiro050
@llvm/issue-subscribers-mlir-core
I think this should be a utility and not a method: it looks like an accessor but has non-trivial cost.
+1, agreed. One way to make the cost more obvious is to make the caller provide the uniquing set as a by-ref argument.
How often does this come up?
I somehow managed to make it this far in my career working on LLVM and always thought that getUsers had the uniquing behavior (LLVM has the same non-uniquing behavior). I'm pretty sure that ~everywhere I wrote getUsers I was expecting uniquing behavior and the code is potentially buggy due to not having that (though it might "happen to work"). Maybe that was just me though and not everyone has that confusion. I think iterating over users is less frequent than uses though, so in absolute terms I think this is somewhat rare.
In Torch-MLIR, out of 10 getUsers calls, I see:
- 4 all_of(getUsers, pred) -- trivially okay
- 5 somehow based on context the users are guaranteed to only use the value once, though often the reasoning is highly nontrivial
- 1 the bug in https://github.com/llvm/torch-mlir/pull/1228
So naively that's 10% of calls to this API resulting in a bug.
Maybe we should rename Value::getUsers to Value::getUseOwners (to make it clear it iterates fundamentally over uses) and add a new Value::getUniqueUsers that takes a uniquing set.
I think iterating over users is less frequent than uses though, so in absolute terms I think this is somewhat rare.
Actually I looked and there are a similar number of getUses calls. Huh