llvm-project icon indicating copy to clipboard operation
llvm-project copied to clipboard

mlir: add getUniqueUsers

Open silvasean opened this issue 2 years ago • 5 comments

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

silvasean avatar Aug 15 '22 23:08 silvasean

@llvm/issue-subscribers-mlir-core

llvmbot avatar Aug 15 '22 23:08 llvmbot

I think this should be a utility and not a method: it looks like an accessor but has non-trivial cost.

joker-eph avatar Aug 16 '22 09:08 joker-eph

+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?

lattner avatar Aug 16 '22 16:08 lattner

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.

silvasean avatar Aug 16 '22 20:08 silvasean

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

silvasean avatar Aug 16 '22 20:08 silvasean