[Intrinsics] Move dest arg functions of MemIntrinsicBase deeper in mem intrinsics hierarchy

[dmakogon]

Summary

Currently we can access destination argument of any memory intrinsic (MemCpyInst, MemSetInst, ...) through the functions of their common base class - MemIntrinsicBase.
But I think these functions must not be declared such early in the class hierarchy (which now for MemSetInst is
IntrinsicInst <-- MemIntrinsicBase<MemIntrinsic> <-- MemIntrinsic <-- MemSetBase<MemWriteIntrinsic> <-- MemSetInst)
We might want to add some new memory intrinsics in the future that do not have dest argument (i.e. sth like memcmp which would have LHS and RHS instead of src and dest).

So this patch introduces new classes in this hierarchy: MemWriteIntrinsicBase in which we define functions related to dest argument; AtomicMemWriteIntrinsic and
MemWriteIntrinsic which derive from MWIB and define classof functions; AnyMemWriteIntrinsic. If now we were to add a new intrinsic which doesn't have dest argument we would derive from MemIntrinsic (or AtomicMemIntrinsic) and wouldn't have that dest argument related logic.
Having introduced these classes, this patch rewrites all uses of MemIntrinsicBase's dest functions so that MemWriteIntrinsicBase is used.

[dmakogon]

Forgot to remove dest from MemTransferBase
Added/edited some comments

[dmakogon]

clang-format

[mkazantsev]

I guess it's an NFC. If so, please add [NFC] in the commit name.

Generally looks good. For sanity check, pls make sure clang builds with your changes.

[dmakogon]

Update uses of MemIntrinsic in polly

[nikic]

This looks okay if we actually want to add new non-write intrinsics in this family (and it makes sense to have them in the same hierarchy). Is there a pending proposal for that?

[mkazantsev]

In D109219#2984715, @nikic wrote:

This looks okay if we actually want to add new non-write intrinsics in this family (and it makes sense to have them in the same hierarchy). Is there a pending proposal for that?

I don't think it has been proposed yet, but we plan to introduce an [element-atomic] memcmp intrinsic. Hierarchy-wise, it is a MemIntrinsic but is not a MemWriteIntrinsic.

Generally, I think this change makes sense even without this notion, just because dest isn't placed where it belongs, and there is no legit way to express a read-only memory intrinsic. So we shouldn't be waiting for proposal details to just do it.

[dmakogon]

Fixed one more use of MemIntrinsic::getDest

[lebedev.ri]

What others said, if there is an upcoming patch to make use of this, then this LG, else NACK.

[dmakogon]

Fixed formatting problems and changed one more use of MemIntrinsic to MemWriteIntrinsic

[dmakogon]

We want to introduce a new memory intrinsic and we need this patch to do that. Please take a look - https://reviews.llvm.org/D109504

[mkazantsev]

Is the provided motivation OK for this refactoring? It's generally good, and I don't really see reasons to hold it away.

[nikic]

In D109219#3003344, @mkazantsev wrote:

Is the provided motivation OK for this refactoring? It's generally good, and I don't really see reasons to hold it away.

A remaining question I have is where the new MemIntrinsic would get actually used. The only commonality between current MemIntrinsics is that they have a destination -- accessing that destination is what MemIntrinsic is used for. After this change, it's not clear to me under what circumstances I could do something useful with MemIntrinsic that would not require a MemWriteIntrinsic (or a newly introduced MemCmpIntrinsic). The only thing MemIntrinsic tells me is that the intrinsic has "something to do with memory". Scrolling through this patch, there seems to be exactly one place where MemIntrinsic is being used (a volatile check) and that use could actually be replaced by MemWriteIntrinsic as well.

[mkazantsev]

In D109219#3003348, @nikic wrote:

In D109219#3003344, @mkazantsev wrote:

Is the provided motivation OK for this refactoring? It's generally good, and I don't really see reasons to hold it away.

A remaining question I have is where the new MemIntrinsic would get actually used. The only commonality between current MemIntrinsics is that they have a destination -- accessing that destination is what MemIntrinsic is used for. After this change, it's not clear to me under what circumstances I could do something useful with MemIntrinsic that would not require a MemWriteIntrinsic (or a newly introduced MemCmpIntrinsic). The only thing MemIntrinsic tells me is that the intrinsic has "something to do with memory". Scrolling through this patch, there seems to be exactly one place where MemIntrinsic is being used (a volatile check) and that use could actually be replaced by MemWriteIntrinsic as well.

For example, we can LICM read-only memory intrinsic with invariant arguments if loop doesn't write (or we've proven non-alias with all writes). We can also CSE them or do whatever else opts we can do to read-only stuff. The most important bit is to have read-only memory intrinsics, and MemoryIntrinsic is just left as their parent because it makes sense.

[dmakogon]

Ping

[xbolva00]

In D109219#3024748, @dmakogon wrote:

Ping

I think you want to “sell” the community new memcmp intrinsic so after that patch is accepted, this one will be basically too.

[mkazantsev]

Byt why can't it go ahead? It's getting hard to sustain it because of multiple changes in different places. Rebasing really becomes pain. This is a (supposedly) NFC, so why not?

[lebedev.ri]

This review may be stuck/dead, consider abandoning if no longer relevant.
Removing myself as reviewer in attempt to clean dashboard.