[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