[PATCH] D47103: Implement strip.invariant.group

[John McCall via Phabricator via llvm-commits]

rjmccall added a comment.

The changes to Clang generally seem reasonable, but I think you should split them into a separate commit from the commit that adds the intrinsic itself.



================
Comment at: clang/lib/CodeGen/CGExpr.cpp:3858
+    }
+  }
+
----------------
Please add a comment explaining why this is necessary.  (I'm actually not sure why it is, because surely the invariant groups we generate don't contain assumptions about memory from fields, right?)


================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:1626
+      Src = Builder.CreateStripInvariantGroup(Src);
+    }
+
----------------
Again, comment.


================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:1767
+    if (CGF.CGM.getCodeGenOpts().StrictVTablePointers)
+      return Builder.CreateLaunderInvariantGroup(IntToPtr);
+
----------------
I think assigning to IntToPtr would be clearer here.  You're changing the value returned, not introducing a reason to early-exit.

And a comment, please, on all of these cases.


Repository:
  rL LLVM

https://reviews.llvm.org/D47103