[PATCH] D38419: Rename instruction classes for memory intrinsics. (NFC)

[Hal Finkel via Phabricator via llvm-commits]

hfinkel added a comment.

In https://reviews.llvm.org/D38419#884898, @dneilson wrote:

> In https://reviews.llvm.org/D38419#884894, @hfinkel wrote:
>
> > In https://reviews.llvm.org/D38419#884869, @dneilson wrote:
> >
> > > In https://reviews.llvm.org/D38419#884865, @hfinkel wrote:
> > >
> > > > In https://reviews.llvm.org/D38419#884799, @dneilson wrote:
> > > >
> > > > > Pulling this, for now. I'm going to make sure that the joint part of the hierarchy (i.e. the AnyMem* classes) will work before trying to land this part.
> > > >
> > > >
> > > > FWIW, I don't really like the PainMem* names, although the AtomicMem* looks like an improvement. Can't you just leave the plain ones alone?
> > >
> > >
> > > I have no particular objection to leaving the plain ones as is. It's what I suggested: http://lists.llvm.org/pipermail/llvm-dev/2017-August/116716.html
> > >
> > > A counter suggestion, that wasn't challenged, was made to change the plain ones as well: http://lists.llvm.org/pipermail/llvm-dev/2017-August/116722.html
> >
> >
> > Fair enough. I challenge :-) -- Sanjoy's comment about silent failures downstream is legitimate, but given how new the atomic versions of these are, I don't think that I'm concerned (and it will fail in the right way: by not recognizing the atomic forms, which is what any current code is probably intended to do (because it was written before the atomic forms existed)).
>
>
> Playing a little devil's advocate... A benefit I can see, off the top of my head, in favour of changing to PlainMem* is cognitive. Having all of PlainMem* & AtomicMem* & AnyMem* makes the programmer have to think, briefly, about whether they're dealing with atomic or non-atomic variants. Leaving the non-atomic variants as is makes it easier to not even consider that the atomic variants exist.


I know. Of course, trying to force people to think in this fashion doesn't always have the desired effect (doing the text substitution and moving on is more likely). It's also true that most people will never see these and we're not really giving people clear guidance on how to update their code. I think we can give relatively-clear guidance, something like, "If your code is looking at Mem* to reason about its effect on memory, then it can probably use AnyMem*, if you're transforming the Mem*, then you probably need to switch to using PlainMem* (or leave it alone, as the case may be). Frankly, I think we'll just need to be diligent in code review for a while regardless of what we do.

I'm also, in general, just not a fan of "Plain" as a prefix. Our memcpy intrinsics (and friends) are already extensions of the underlying C functions (we can specify alignments, volatileness, etc.). If we really want to rename in this way, I think that using NonAtomic as the prefix is better.


https://reviews.llvm.org/D38419