[PATCH] D140647: Handle simple diamond CFG hoisting in DivRemPairs.

[Nikita Popov via Phabricator via llvm-commits]

nikic added a subscriber: nickdesaulniers.
nikic added a comment.

Thanks for the update. I have two more minor suggestions:

1. I believe we are missing negative tests for the case where either a) the div or rem block don't have a unique predecessor or b) the common predecessor has other successors. (Please let me know if I just missed them, there's a lot of test files...)

2. I would suggest a code comment on why we are doing this transform, because the motivation here is somewhat different than for all the other cases this pass handles (in other cases, we end up not executing a div/rem operation on some code path, while here we have a scheduling / code size optimization).

In addition to that, I believe this transform has multiple pre-existing correctness issues. These are kind of orthogonal to your patch, but I noticed them while reviewing the code:

1. If the terminator of the predecessor is not guaranteed to transfer, then we might hoist a dynamically dead div/rem, causing undefined behavior. In particular, this can happen with an invoke of a non-willreturn function.

2. If the predecessor is a catchswitch pad, it is not legal to hoist into it.

3. With the pending changes to callbr semantics (https://reviews.llvm.org/D135997), if callbr defines one of the div/rem operands, it may not be available after hoisting. This is not a problem //yet// though, so this is mostly something for @nickdesaulniers to keep an eye on. This might be a problem in other transforms as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140647