[PATCH] D31389: [ARM] Remove a dead ADD during the creation of TBBs

[Renato Golin via Phabricator via llvm-commits]

rengolin added inline comments.


================
Comment at: lib/Target/ARM/ARMConstantIslandPass.cpp:2033
+      RemovableAdd = &*I;
+    else if (RemovableAdd) {
+      for (unsigned K = 0, E = I->getNumOperands(); K != E; ++K) {
----------------
I'm confused. This means the `else` part will only execute one iteration after the add is detected.

Assuming ADD is never the last instruction (because the last is JumpMI), this should be ok, but I'm not sure why it's necessary, and it makes the code flow harder to understand.

Also, if there are multiple adds, this would overwrite the `RemovableAdd` pointer *before* it was sure the new `ADD` can be removed.

I'd make the whole iteration in one go, on a local pointer, and only update the higher scope pointer once the add is really the one we're looking for.

Would it be an error if we find two adds in the right condition?


https://reviews.llvm.org/D31389