This is an archive of the discontinued LLVM Phabricator instance.

[ValueTracking] Enable -branch-on-poison-as-ub by default
ClosedPublic

Authored by nikic on May 10 2022, 3:29 AM.

Details

Summary

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.

Diff Detail

Event Timeline

nikic created this revision.May 10 2022, 3:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 10 2022, 3:29 AM
nikic requested review of this revision.May 10 2022, 3:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 10 2022, 3:29 AM
xbolva00 added inline comments.
llvm/test/Transforms/VectorCombine/AArch64/load-extractelement-scalarization.ll
680–681

Remove todo? Or is it still valid?

fhahn added a comment.May 10 2022, 4:35 AM

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

reames requested changes to this revision.May 17 2022, 8:42 AM

Marking so it drops off my review queue until noted issues in other passes are fixed.

This revision now requires changes to proceed.May 17 2022, 8:42 AM
nikic added a comment.May 18 2022, 6:47 AM

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

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.

fhahn added a comment.May 18 2022, 7:38 AM

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

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.

nikic added a comment.EditedMay 18 2022, 7:55 AM

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

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.

also Polly.DeLICM::pr41656.ll fails in precommit CI.

nikic added a comment.May 24 2022, 2:30 AM

I believe https://reviews.llvm.org/D125898 will fix the last known issue.

nikic updated this revision to Diff 432898.May 30 2022, 5:57 AM
nikic edited the summary of this revision. (Show Details)

Rebase

reames accepted this revision.May 31 2022, 9:56 AM

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.

This revision is now accepted and ready to land.May 31 2022, 9:56 AM
fhahn accepted this revision.May 31 2022, 10:19 AM

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.

This revision was landed with ongoing or failed builds.Jun 1 2022, 1:46 AM
This revision was automatically updated to reflect the committed changes.
nikic added a comment.Jun 2 2022, 5:44 AM

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.