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

Tue Jul 19 03:07:45 PDT 2022

fhahn added a comment.

In D129370#3644789 <https://reviews.llvm.org/D129370#3644789>, @chill wrote:

> In D129370#3638912 <https://reviews.llvm.org/D129370#3638912>, @nikic wrote:
>
>> 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.
>
> Thanks for the review, I think I have fixed those issues.

It looks like we are missing test-coverage for at least some of those case. Could you add extra tests to cover the checks you added?

================
Comment at: llvm/test/Transforms/SimplifyCFG/hoist-common-skip.ll:75
+entry:
+  %0 = load i16, ptr %d, align 2
+  %conv = sext i16 %0 to i32
----------------
the test case can be further simplified, e.g. there's no need to load & compare here. instead the condition can simplify be passed as argument.

Similarly, there's no need to return an `i32` from the function or have multiple diverging instructions in the blocks.

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

https://reviews.llvm.org/D129370