[PATCH] D17337: __builtin_object_size problem with pointer as argument

[George Burgess IV via llvm-commits]

george.burgess.iv added a comment.

Looks good overall -- just a few small comments for you.


================
Comment at: lib/Transforms/InstCombine/InstructionCombining.cpp:1944
@@ -1943,3 +1943,3 @@
   if (isAllocSiteRemovable(&MI, Users, TLI)) {
     for (unsigned i = 0, e = Users.size(); i != e; ++i) {
       Instruction *I = cast_or_null<Instruction>(&*Users[i]);
----------------
Can we have a small comment here explaining why we iterate through `Users` twice? Something like "We must lower all `@llvm.objectsize` calls first, because they may use a bitcast/GEP of the alloca we're removing" might be helpful.

================
Comment at: lib/Transforms/InstCombine/InstructionCombining.cpp:1957
@@ +1956,3 @@
+          eraseInstFromFunction(*I);
+          Users[i] = nullptr; // Skip examing this in next loop.
+        }
----------------
Nit: Can we move this somewhere above `replaceInstUsesWith`? Having the RAUW update `Users[i]` seems pointless if we're just going to null it out anyway.

================
Comment at: lib/Transforms/InstCombine/InstructionCombining.cpp:1957
@@ +1956,3 @@
+          eraseInstFromFunction(*I);
+          Users[i] = nullptr; // Skip examing this in next loop.
+        }
----------------
george.burgess.iv wrote:
> Nit: Can we move this somewhere above `replaceInstUsesWith`? Having the RAUW update `Users[i]` seems pointless if we're just going to null it out anyway.
Nit: The comment should say "Skip examining" :)


Repository:
  rL LLVM

http://reviews.llvm.org/D17337