[PATCH] D76050: [LoopPeel] Turn incorrect assert into a check

Thu Mar 12 05:13:54 PDT 2020

fhahn added inline comments.

================
Comment at: llvm/test/Transforms/LoopUnroll/wrong_assert_in_peeling.ll:77
+bb2:                                              ; preds = %bb12, %bb1
+  %tmp3 = phi i32 [ undef, %bb1 ], [ %tmp4, %bb12 ]
+  %tmp4 = add nsw i32 %tmp3, %tmp
----------------
mkazantsev wrote:
> fhahn wrote:
> > are the undef here and in other places required for the test case? Otherwise I think it would be better to replace them with constants/regular values to make the test case more robust. Same for branch on constants.
> We have quite a lot tests using undef and afaik there is no problems with their robustness.
It is not a huge problem (and depends on case by case).

But this test contains plenty of presumably unnecessary (conditional) branches on constants/undef, presumably unnecessary calls to llvm.experimental.deoptimize, branches on compare with undef. IMO this makes the test case unnecessarily verbose (and harder to focus on the actual  problem it tests for). Also lots of the sub/icmp instructions are at the risk of being completely folded away in the future, e.g. if loop unrolling cost modeling would exploit undef more.

IIUC to trigger the assertions, I think all that would be required is a loop nest where we try to check a condition involving an expression with the inner and outer induction variable with the property mentioned in the description.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76050