Page MenuHomePhabricator

[LSR] Drop poison flags for post-inc IVs (PR46943)
Needs ReviewPublic

Authored by pnkfelix on Mon, Nov 16, 1:09 PM.
This revision needs review, but there are no reviewers specified.

Details

Reviewers
None
Summary

Includes generalized regression test that puts icmp and cond br in distinct
blocks, and an update to an existing test to reflect correction to LSR
transformation.

Diff Detail

Unit TestsFailed

TimeTest
440 mslinux > HWAddressSanitizer-x86_64.TestCases::sizes.cpp
Script: -- : 'RUN: at line 3'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang --driver-mode=g++ -m64 -gline-tables-only -fsanitize=hwaddress -fuse-ld=lld -mcmodel=large -mllvm -hwasan-globals -mllvm -hwasan-use-short-granules -mllvm -hwasan-instrument-landing-pads=0 -mllvm -hwasan-instrument-personality-functions /mnt/disks/ssd0/agent/llvm-project/compiler-rt/test/hwasan/TestCases/sizes.cpp -nostdlib++ -lstdc++ -o /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/test/hwasan/X86_64/TestCases/Output/sizes.cpp.tmp
60 mslinux > SanitizerCommon-Unit._/Sanitizer-x86_64-Test::SanitizerCommon.StackDepotPrint
Note: Google Test filter = SanitizerCommon.StackDepotPrint [==========] Running 1 test from 1 test case. [----------] Global test environment set-up.

Event Timeline

pnkfelix created this revision.Mon, Nov 16, 1:09 PM
Herald added a project: Restricted Project. · View Herald TranscriptMon, Nov 16, 1:09 PM
pnkfelix requested review of this revision.Mon, Nov 16, 1:09 PM
pnkfelix added inline comments.Mon, Nov 16, 1:51 PM
llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp
2401

thanks lint! (old habits die hard.) will fix.

nikic retitled this revision from Fix for LLVM bug 46943. to [LSR] Drop poison flags for post-inc IVs (PR46943).Tue, Nov 17, 11:48 AM
nikic added a subscriber: nikic.Tue, Nov 17, 11:53 AM

The approach here doesn't look right to me. Poison flags have no relation to the position where a certain instruction is executed. What's relevant is where the instruction is used.

This transform adds a new poison-is-UB user to the post-inc IV. Whereever that code is, should be the place that drops poison flags. The code is pretty convoluted though, so I didn't immediately spot where that happens.

The approach here doesn't look right to me. Poison flags have no relation to the position where a certain instruction is executed. What's relevant is where the instruction is used.

This transform adds a new poison-is-UB user to the post-inc IV. Whereever that code is, should be the place that drops poison flags. The code is pretty convoluted though, so I didn't immediately spot where that happens.

The main instance I know of of a new poison-is-UB user is the TermBr itself that will immediately succeed Cond after the transformation. Based on what the transformation is doing, I guess *all* users of the Cond are potentially poison-is-UB users.

That is, for the specific bug that I'm recreating: (1.) the addition now happens before the Cond instead of after (and still overflows; it always overflowed), (2.) the poison value flows into the Cond, yielding poison, and (3.) the poison from the Cond flows into the TermBr, yielding UB.

I considered starting from the TermBr, and tracing backwards to find every operation that flows into its computation. But I was concerned that would be overly conservative and potentially introduce regressions. I considered the change I posted to be the simplest change I could make that would still fix the bug.

I suppose I could try to trace the value-flow backwards from the TermBr, but only remove the nowrap flags from the instructions that are *dominated* by the Cond... would that be preferable to you, @nikic ?

aqjune added a subscriber: aqjune.Fri, Nov 27, 2:14 PM