Now that SimpleLoopUnswitch and other transforms no longer introduce branch on poison, enable the -branch-on-poison-as-ub option by default. The practical impact of this is mostly better flag preservation in SCEV, and some freeze instructions no longer being necessary.
Details
Diff Detail
Event Timeline
llvm/test/Transforms/VectorCombine/AArch64/load-extractelement-scalarization.ll | ||
---|---|---|
680–681 | Remove todo? Or is it still valid? |
Sounds good to me in general. I think there are at least 2 known issues with SimplifyCFG & LoopUnroll introducing branches on poison/undef. Linked to https://github.com/llvm/llvm-project/issues/45489. If you know of other issues in that area it might be good to all link them to that issue, so it is easier to keep track of the outstanding issues
Marking so it drops off my review queue until noted issues in other passes are fixed.
ControlHeightReduction and LoopUnroll have been fixed, patches for IndVarSimplify (https://reviews.llvm.org/D124910), JumpThreading (https://reviews.llvm.org/D125869) and CodeGenPrepare (https://reviews.llvm.org/D125887) are pending.
It looks like the following LoopUnroll tests still fail verification:
Failed Tests (4): LLVM :: Transforms/LoopUnroll/loop-remarks.ll LLVM :: Transforms/LoopUnroll/peel-loop-and-unroll.ll LLVM :: Transforms/LoopUnroll/runtime-loop-multiexit-dom-verify.ll LLVM :: Transforms/LoopUnroll/runtime-loop-multiple-exits.ll
I still need to check what the actual issue is.
Ah, I think this is due to the branch on lcmp.mod that also gets introduced (for the epilogue case). I'll take a look.
LGTM
Thank you for methodically working through all the issues found in the tests.
For the record, I think there's a very good chance that we end up reverting this change. I will not be surprised at all to see failures reported because we missed some transform which had insufficient test coverage in tree. Despite that, I think it's time to move this forward and start smoking out any remaining issues.
I do request that you try to land this at an off time. Please pick a window where builbots can cycle quickly, and we can avoid having this end up in large build requests.
LGTM, thanks!
AFAIK all known issues should be fixed. Let's fix uncovered issues as we go along.
I *think* there are a couple of places where transformations are not performed with MSan because it also checks for branches on undef values. Might be good to remove those if possible now.
For the record, this caused a large compile-time regression on lencod: http://llvm-compile-time-tracker.com/compare.php?from=299baac64da32ed995098c43f99091dfd2694699&to=03aceab08bc9439e454d827bd1ffc94659c2c510&stat=instructions From what I can tell, this is because we now determine the trip count of some loops and fully unroll them. This is probably another case of too aggressive unrolilng, but I don't think it indicates a problem with this patch itself.
Remove todo? Or is it still valid?