[PATCH] D89632: [DomTree] Accept Value as Def (NFC)

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

In D89632#2348338 <https://reviews.llvm.org/D89632#2348338>, @asbirlea wrote:

> Allowing the `dominates` call to accept any `Value` can lead to unexpected errors, because there can be classes that are derived from `Value` that have nothing to do with the IR or the DT in some cases, and for those the result will always be true instead of an error or an assert. Pruning all such cases to include in the internal assert also doesn't make sense to me.
>
> For example, all `MemoryAccess`s in MemorySSA are `Value`s, but their use-user chains are isolated to their own "island" (a use of a MemoryAccess must be another MemoryAccess). A DT.dominate() call with a MemoryAccess as the first argument wouldn't make any sense, and should not be allowed, but the new API will accept it and return true.
> Again, this is just an example, and many more may be added in the future, so it wouldn't make sense to assert inside the DT `dominate` call the `DefV` is not a `MemoryAccess`.
>
> I think a better option is to add APIs for some of the specific cases you're looking for, for example a `Constant` (`GlobalValue` is subclassed already) or an `Argument`.
> Even a generic API for Value should check for the specific cases known to be ok and return true for those, and then assert for anything else.

APIs for specific cases don't really make sense to me: If I know in advance that something is an Argument or Constant, there's no point in performing a domination check, as the result is known in advance. The point of accepting a Value* here is that you can pick an arbitrary IR value and perform a dominance check on it, without having to perform dyn_casts in advance.

Of course, this is only meaningful for values that appear as normal instruction operands, i.e. other instructions, arguments and constants. Using this with anything else is not meaningful, and I do agree that the assertion should reflect that. I've adjusted it to check for Constant/Argument only in https://github.com/llvm/llvm-project/commit/c0e8c94373b4e97d2eecc53bb16d22e085b73948.

With the stricter assertion, does this seem reasonable to you? We have many APIs that accept Value*s, and I daresay that nearly all of them wouldn't know what to do with a MemoryAccess*, so I don't see this as a fundamental problem as long as such arguments are properly rejected rather than silently misbehaving.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89632/new/

https://reviews.llvm.org/D89632