[PATCH] D129370: [SimplifyCFG] Allow SimplifyCFG hoisting to skip over non-matching instructions

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

I think you need to be more careful about the side effects here. You generally need to distinguish control flow effects from memory effects. For example, if you skip over an instruction that is not must-exec (i.e. not nothrow or not willreturn), and then want to hoist a udiv (which is side-effect free, but not generally speculatable), then that is illegal and I don't think you currently handle this correctly.

On a more general note, this skipping of non-identical instructions in lock-step seems kind of ... random? It isn't obvious to me why we would typically expect there to be sequences of different instructions that just happen to have the same length.



================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:1446
+  return I->mayThrow() || I->mayWriteToMemory();
+}
+
----------------
Why can Instruction::mayHaveSideEffects() not be used?


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:1456
+// If we have seen an instruction with side effects, it's unsafe to reorder an
+// instruction which reads memory or itself has side effects.
+  if (AfterSideEffect && (I->mayReadOrWriteMemory() || hasSideEffects(I)))
----------------
Wrong comment indentation here and below.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:1463
+// single-predecessor basic blocks only, then it's sufficient to know the
+// instruction operands are not in the same basic block as the instruction.
+  for (Value *Op : I->operands()) {
----------------
What does this have to do with the single predecessor? Doesn't this generally hold?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129370/new/

https://reviews.llvm.org/D129370