[llvm] r314435 - [JumpThreading] Preserve DT and LVI across the pass

Fri Sep 29 10:13:04 PDT 2017

Hello Philip,

While developing the preservation of DT across JT we were asked by Davide
Italiano to see if we could also preserve LVI. When we did so we discovered
a correctness bug in test Transforms/JumpThreading/pr33917.ll:

FAIL: LLVM :: Transforms/JumpThreading/pr33917.ll (19407 of 22252)

******************** TEST 'LLVM :: Transforms/JumpThreading/pr33917.ll'
FAILED ********************

Script:

--

/work/brzycki/upstream/build/x86_e73dbb2/./bin/opt -jump-threading
-correlated-propagation /work/brzycki/upstream/llvm-pr
oject/llvm/test/Transforms/JumpThreading/pr33917.ll -S | /work/brzycki
/upstream/build/x86_e73dbb2/./bin/FileCheck/work/brzycki/upstream/llvm-pro
ject/llvm/test/Transforms/JumpThreading/pr33917.ll

--

Exit Code: 1

Command Output (stderr):

--

/work/brzycki/upstream/llvm-project/llvm/test/Transforms/JumpThreading/pr33917.ll:24:15:
error: expected string not found in input

; CHECK-NEXT: [[T11:%.*]] = icmp ne i8* [[T9]], null

              ^

<stdin>:23:7: note: scanning from here

good: ; preds = %bb9

      ^

<stdin>:23:7: note: with variable "T9" equal to "%t9"

good: ; preds = %bb9

      ^

<stdin>:25:2: note: possible intended match here

%cond = icmp eq i64 %t12, 1

^

Inspection of the .ll showed this optimization to be invalid. Further debug
showed the culprit to be caused by LVI's conditional requirement of DT:

[2017-09-29 11:51:08.391923791] 0x25020a0   Executing Pass 'Function Pass
Manager' on Module '/work/brzycki/upstream/llvm-
project/llvm/test/Transforms/JumpThreading/pr33917.ll'...

[2017-09-29 11:51:08.392083604] 0x2514e00     Executing Pass 'Dominator
Tree Construction' on Function 'patatino'...

[2017-09-29 11:51:08.392201600] 0x2514e00     Executing Pass 'Basic Alias
Analysis (stateless AA impl)' on Function 'patatino'...

[2017-09-29 11:51:08.392328316] 0x2514e00     Executing Pass 'Function
Alias Analysis Results' on Function 'patatino'...

[2017-09-29 11:51:08.392402761] 0x2514e00     Executing Pass 'Lazy Value
Information Analysis' on Function 'patatino'...

[2017-09-29 11:51:08.392431145] 0x2514e00     Executing Pass 'Jump
Threading' on Function 'patatino'...

[2017-09-29 11:51:08.393124549] 0x2514e00      Freeing Pass 'Dominator Tree
Construction' on Function 'patatino'...

[2017-09-29 11:51:08.393155519] 0x2514e00      Freeing Pass 'Jump
Threading' on Function 'patatino'...

[2017-09-29 11:51:08.393173763] 0x2514e00      Freeing Pass 'Basic Alias
Analysis (stateless AA impl)' on Function 'patatino'...

[2017-09-29 11:51:08.393190916] 0x2514e00      Freeing Pass 'Function Alias
Analysis Results' on Function 'patatino'...

[2017-09-29 11:51:08.393208870] 0x2514e00     Executing Pass 'Value
Propagation' on Function 'patatino'...

[2017-09-29 11:51:08.393528791] 0x2514e00     Made Modification 'Value
Propagation' on Function 'patatino'...

[2017-09-29 11:51:08.393566180] 0x2514e00      Freeing Pass 'Value
Propagation' on Function 'patatino'...

[2017-09-29 11:51:08.393584428] 0x2514e00      Freeing Pass 'Lazy Value
Information Analysis' on Function 'patatino'...

[2017-09-29 11:51:08.393618018] 0x2514e00     Executing Pass 'Module
Verifier' on Function 'patatino'...

[2017-09-29 11:51:08.393809687] 0x2514e00      Freeing Pass 'Module
Verifier' on Function 'patatino'...

[2017-09-29 11:51:08.393833337] 0x25020a0   Made Modification 'Function
Pass Manager' on Module '/work/brzycki/upstream/llvm-
project/llvm/test/Transforms/JumpThreading/pr33917.ll'...

[2017-09-29 11:51:08.393852785] 0x25020a0    Freeing Pass 'Assumption Cache
Tracker' on Module '/work/brzycki/upstream/llvm-
project/llvm/test/Transforms/JumpThreading/pr33917.ll'...

[2017-09-29 11:51:08.393890918] 0x25020a0    Freeing Pass 'Target Library
Information' on Module '/work/brzycki/upstream/llvm-
project/llvm/test/Transforms/JumpThreading/pr33917.ll'...

[2017-09-29 11:51:08.393908905] 0x25020a0   Executing Pass 'Print Module
IR' on Module '/work/brzycki/upstream/llvm-project/llvm/test/Transforms/
JumpThreading/pr33917.ll'...

; ModuleID = '/work/brzycki/upstream/llvm-project/llvm/test/Transforms/
JumpThreading/pr33917.ll'

In this trace of the pass manager you can see the following situation:

DT executed

LVI executed (and using the previously constructed DT)

JT executed

Freeing of DT

Freeing of JT

Value Propagation executed

Freeing of VP

Freeing of LVI

The problem here is LVI was not re-generated before Value Propagation
executed. This means that the pointer LVI had to DT is now garbage (it was
freed after executing JT).

This is why we explicitly added the requirement for DT
inside CorrelatedValuePropagation.cpp. This additional requirement informs
the pass manager it cannot free DT after executing JT.

To the best of my knowledge LVI is not consistent and preserved. I am
stating this cautiously because  JT and VP are the only users of LVI today
and they are consistent. What I do not know is if another pass were added
that required LVI and not DT would the pass manager correctly execute/free
the passes.

In my opinion DT should be required by LVI to remove this potential
landmine. When examining the history of the LVI pass I inferred from the
commits that DT was made conditional due to the complexities of adding DT
preservation to JT. With Jakub's simplified DT api and our work in using it
I think it makes sense to enable LVI's requirement on DT. It would simplify
the logic in the pass and prevent these transitive dependencies for the
Pass manager to miss.

On Fri, Sep 29, 2017 at 11:03 AM, Philip Reames via llvm-commits <
llvm-commits at lists.llvm.org> wrote:

> To be clear, you're saying that there was a bug, we worked around it by
> making jump-threading not preserve LVI/DT, fixed the bug, then re-enabled
> the preservation?  If so, that's fine.  It would have been very helpful if
> the submit comment mentioned that history.
>
>
>
> On 09/29/2017 08:49 AM, Sebastian Pop wrote:
>
>> On Fri, Sep 29, 2017 at 10:29 AM, Philip Reames via llvm-commits
>> <llvm-commits at lists.llvm.org> wrote:
>>
>>> Er, have you checked the history here?  I thought this preservation was
>>> removed relatively recently to avoid a correctness bug.
>>>
>> I think the bug you mention was related to jump-threading not preserving
>> LVI,
>> and this is because LVI was computed by jump-threading
>> and LVI relies on correct dominators.
>> I believe that with the committed patch we addressed all the problems
>> to preserve both dominators and LVI across jump-threading.
>> Please let us know if you see other problems.
>>
>> Thanks,
>> Sebastian
>>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170929/cb973f38/attachment.html>