[PATCH] D51327: [MemorySSA] Add expesive check for validating clobber accesses.
George Burgess IV via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Aug 27 15:31:25 PDT 2018
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
More information about the llvm-commits
mailing list