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

[Justin Lebar via Phabricator via llvm-commits]

jlebar added a comment.

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).`



================
Comment at: include/llvm/CodeGen/SelectionDAGNodes.h:69
 class Value;
+class AtomicSDNode; //forward declare for type check
 
----------------
Nit: Is this comment worth the cost?


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

https://reviews.llvm.org/D57596