[PATCH] D57596: [CodeGen] Be conservative about atomic accesses as for volatile

Philip Reames via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Feb 1 11:20:35 PST 2019


reames planned changes to this revision.
reames added a comment.

In D57596#1381092 <https://reviews.llvm.org/D57596#1381092>, @jlebar wrote:

> My main comment is, in most or all of these places where we s/`isVolatile`/`isVolatile && isAtomic`/, it's not going to be obvious to a reader whether isAtomic is really necessary, or just part of a conservative check.
>
> I see that you plan to go back over these, and that's cool, but I wonder if we should leave a breadcrumb that indicates that you plan to come back to this.  That way if you miss a spot or whatever, readers won't think that isAtomic is somehow deeply meaningful (and won't have to scour through blame to find this comment I'm leaving now... :).
>
> Even something as simple as `//TODO(reames): Figure out whether isAtomic is really necessary (see D57601).`


Sure.  I won't include the name since that's not idiomatic, but I'll add comments and update.


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

https://reviews.llvm.org/D57596





More information about the llvm-commits mailing list