This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by dmakogon on Sep 3 2021, 3:02 AM.

Details

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.

Diff Detail

Event Timeline

dmakogon created this revision.Sep 3 2021, 3:02 AM
dmakogon requested review of this revision.Sep 3 2021, 3:02 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 3 2021, 3:02 AM
dmakogon updated this revision to Diff 370524.Sep 3 2021, 3:05 AM

Forgot to remove dest from MemTransferBase
Added/edited some comments

dmakogon edited the summary of this revision. (Show Details)Sep 3 2021, 3:21 AM
dmakogon set the repository for this revision to rG LLVM Github Monorepo.
dmakogon edited the summary of this revision. (Show Details)
dmakogon edited the summary of this revision. (Show Details)
dmakogon edited the summary of this revision. (Show Details)
dmakogon updated this revision to Diff 370537.Sep 3 2021, 4:06 AM

clang-format

dmakogon edited the summary of this revision. (Show Details)Sep 3 2021, 4:07 AM

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 updated this revision to Diff 370846.Sep 5 2021, 10:29 PM

Update uses of MemIntrinsic in polly

nikic added a subscriber: nikic.Sep 6 2021, 1:13 AM

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 added a comment.EditedSep 6 2021, 10:01 PM

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 updated this revision to Diff 371025.Sep 7 2021, 3:37 AM

Fixed one more use of MemIntrinsic::getDest

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

dmakogon updated this revision to Diff 371563.Sep 9 2021, 5:51 AM

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

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

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

nikic added a comment.Sep 16 2021, 2:02 AM

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 added a comment.EditedSep 17 2021, 12:25 AM

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 edited the summary of this revision. (Show Details)Oct 5 2021, 8:52 AM
dmakogon added a reviewer: apilipenko.

Ping

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

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?

dmakogon planned changes to this revision.Oct 12 2021, 11:11 PM
ormris removed a subscriber: ormris.Jan 24 2022, 11:36 AM
lebedev.ri resigned from this revision.Jan 12 2023, 4:57 PM

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

Herald added a project: Restricted Project. · View Herald TranscriptJan 12 2023, 4:57 PM