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

[Alina Sbirlea via Phabricator via llvm-commits]

asbirlea added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89632