[PATCH] D51327: [MemorySSA] Add expesive check for validating clobber accesses.

[George Burgess IV via Phabricator via llvm-commits]

george.burgess.iv added a comment.

Thanks for this!



================
Comment at: lib/Analysis/MemorySSA.cpp:456
 
+static void LLVM_ATTRIBUTE_UNUSED
+checkClobberSanityInstruction(Instruction *I, const MemorySSA &MSSA) {
----------------
`LLVM_ATTRIBUTE_UNUSED` is probably unneeded; we have a member function that calls this regardless of whether NDEBUG/EXPENSIVE_CHECKS are on


================
Comment at: lib/Analysis/MemorySSA.cpp:463
+          dyn_cast_or_null<MemoryUseOrDef>(MSSA.getMemoryAccess(I))) {
+    auto *Clobber = MSSA.getWalker()->getClobberingMemoryAccess(I);
+    UpwardsMemoryQuery Q(I, UseOrDef);
----------------
Is it reasonable to not involve the walker here at all? :)

I ask because, if EXPENSIVE_CHECKS are on, the walker should already be calling checkClobberSanity on everything it hands back. So I think the only coverage we need out of this "we have these accesses that report `isOptimized() == true`. Let's make sure that the optimization is valid." (which it might have been originally, but then the IR+MemorySSA were changed and now it's not)


================
Comment at: lib/Analysis/MemorySSA.cpp:1710
+/// Check sanity of the clobbering instruction for I.
+void MemorySSA::checkClobberSanityInstruction(Instruction *I) {
+  ::checkClobberSanityInstruction(I, *this);
----------------
This appears unused?


================
Comment at: lib/Analysis/MemorySSA.cpp:1716
+void MemorySSA::checkClobberSanityAccess(const MemoryAccess *MA) const {
+  if (auto *MUD = dyn_cast<MemoryUseOrDef>(MA))
+    ::checkClobberSanityInstruction(MUD->getMemoryInst(), *this);
----------------
It seems a bit awkward that we're casting this to a MUD, then extracting the instruction, then looking the MUD back up, then casting it to a MUD (in `checkClobberSanityInstruction`).

Since this is the only user of `checkClobberSanityInstruction`, would it make sense to manually inline it in this function and simplify a bit?


Repository:
  rL LLVM

https://reviews.llvm.org/D51327