Page MenuHomePhabricator

[ValueTracking] Branch on poison is UB, branch cond is non-poison.
Needs RevisionPublic

Authored by fhahn on May 30 2020, 1:03 PM.

Details

Summary

According to LangRef, branching on a poison condition is UB, hence the
condition should be guaranteed to not be poison.

This allows us to deduce additional nsw/nuw in some cases.

The changes in test/Transforms/LoopVectorize/consecutive-ptr-uniforms.ll
are required because add nuw %i, -1 means %i can only be 0.

The changes in test/Transforms/LoopUnroll/runtime-small-upperbound.ll
are required, because previously we had a max trip count of 4 and 6
respectively (instead of 3 and 5), because SCEV assumed the additions in
the loops could overflow and the initial value could be just below UMAX,
with the first add overflowing. I also tried to update the test to cover
all possible combinations.

Diff Detail

Unit TestsFailed

TimeTest
600 msPolly.DeLICM::Unknown Unit Message ("")
Script: -- : 'RUN: at line 1'; opt -polly-process-unprofitable -polly-remarks-minimal -polly-use-llvm-names -polly-import-jscop-dir=/mnt/disks/ssd0/agent/workspace/BETA_amd64_debian_testing_clang8/llvm-project/polly/test/DeLICM -polly-codegen-verify -polly-delicm -analyze < /mnt/disks/ssd0/agent/workspace/BETA_amd64_debian_testing_clang8/llvm-project/polly/test/DeLICM/pr41656.ll | FileCheck /mnt/disks/ssd0/agent/workspace/BETA_amd64_debian_testing_clang8/llvm-project/polly/test/DeLICM/pr41656.ll

Event Timeline

fhahn created this revision.May 30 2020, 1:03 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 30 2020, 1:03 PM
nikic added a subscriber: nikic.May 30 2020, 1:28 PM
efriedma accepted this revision.May 30 2020, 6:30 PM

This has rather more impact than I was expecting... but I guess this sort of reasoning was missing before, to some extent.

LGTM

This revision is now accepted and ready to land.May 30 2020, 6:30 PM
efriedma requested changes to this revision.May 30 2020, 7:07 PM

Actually, thinking about it a bit more, I'm not completely comfortable with landing this yet. We still have a few known issues with IR transforms that don't correctly freeze branch conditions, and I'm afraid the interaction here might expose them as miscompiles.

This revision now requires changes to proceed.May 30 2020, 7:07 PM

The passes we are aware that introduce branch on poison are: IndVarSimplify, LoopUnswitch, SimpleLoopUnswitch, and SimpifyCFG.
(https://web.ist.utl.pt/nuno.lopes/alive2/index.php?hash=4beb2b117e2fdd2c)

I tend to agree with Eli that this patch may help triggering end-to-end miscompilations. But it's otherwise correct; nice catch!

fhahn added a comment.May 31 2020, 5:43 AM

Actually, thinking about it a bit more, I'm not completely comfortable with landing this yet. We still have a few known issues with IR transforms that don't correctly freeze branch conditions, and I'm afraid the interaction here might expose them as miscompiles.

Sounds good to me! I've filed https://bugs.llvm.org/show_bug.cgi?id=46144 to collect/keep information of the passes that need fixing. I guess it's fine to wait with landing this patch until most issues have been resolved.