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

Fri Sep 29 10:14:38 PDT 2017

My apologies, I meant to say:
To the best of my knowledge LVI is *now* consistent and preserved. I

On Fri, Sep 29, 2017 at 12:13 PM, Brian M. Rzycki <brzycki at gmail.com> wrote:

> 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/wor
> k/brzycki/upstream/llvm-project/llvm/test/Transforms/JumpThr
> eading/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-p
> roject/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-p
> roject/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-p
> roject/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-p
> roject/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/Ju
> mpThreading/pr33917.ll'...
>
> ; ModuleID = '/work/brzycki/upstream/llvm-project/llvm/test/Transforms/Ju
> mpThreading/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/93c34ea3/attachment.html>