Page MenuHomePhabricator

Remove insertDebugValuesForPHIs() from LCSSA to prevent assignments from being reordered
ClosedPublic

Authored by n-omer on Dec 3 2020, 7:17 AM.

Details

Summary

BugZilla: https://bugs.llvm.org/show_bug.cgi?id=48206

The LCSSA pass makes use of a function insertDebugValuesForPHIs() to propogate dbg.value() intrinsics to newly inserted PHI instructions.

Faulty behaviour occurs when the associated parent PHI of a newly inserted PHI is not the most recent assignment to a source variable as can be seen below:

; input.ll:
%S2 = ...
br label %loop.interior
loop.interior:
    br i1 %S2, label %if.true, label %if.false                    
if.true:
    %X1 = ...
    @llvm.dbg.value(var = %X1)  
    br label %post.if

if.false:                         
    %X2 = ...
    @llvm.dbg.value(var = %X2)  
    br label %post.if
    
post.if:
    %X3 = phi(X1, X2)            
    @llvm.dbg.value(var = %X3)  
    
    %Y1 = ...                    
    @llvm.dbg.value(var = %Y1)
    
    br i1 %S2, label %loop.exit, label %loop.interior

loop.exit:
    ... = X3 + 4


; output.ll:
%S2 = ...
br label %loop.interior
loop.interior:
    br i1 %S2, label %if.true, label %if.false                       
if.true:
    %X1 = ...
    @llvm.dbg.value(var = %X1)  
    br label %post.if
    
if.false:                         
    %X2 = ...
    @llvm.dbg.value(var = %X2)  
    br label %post.if
    
post.if:
    %X3 = phi(X1, X2)            
    @llvm.dbg.value(var = %X3)  
    
    %Y1 = ...                    
    @llvm.dbg.value(var = %Y1)
    
    br i1 %S2, label %loop.exit, label %loop.interior

loop.exit:
    %X3.lcssa = phi(X3)
    @llvm.dbg.value(var = %X3.lcssa) <---- Incorrect!
    %X4 = %X3.lcssa + 3

In the pseudo-IR above, insertDebugValuesForPHIs() propogates the incorrect dbg.value() intrinsic. The intrinsic that should be propogated is the one associated with %Y1 because that is the most recent assignment to var, but it is simply ignored. This results in incorrect debugging information as it is effectively re-ordering assignments.

This change removes the call to insertDebugValuesForPHIs() from LCSSA, preventing incorrect dbg.value intrinsics from being propagated. As you can see in the locstats output below (generated by building Clang 3.4 with RelWithDebInfo with and without my change), even though the number of variables in the 100% bucket has gone down, the number of variables in the 0% bucket has also gone down. Apart from the fact that coverage has increased, incorrect debug information has also been removed (as seen in the example above). This is because even though LCSSA no longer inserts debug intrinsics for new PHIs, not inserting an intrinsic at all allows the last dominating dbg.value to continue dominating later instructions which results in the correct behaviour.

Before:

=================================================
           Debug Location Statistics
=================================================
    cov%           samples         percentage(~)
-------------------------------------------------
  0%               841882               25%
  (0%,10%)          41547                1%
  [10%,20%)         49777                1%
  [20%,30%)         49478                1%
  [30%,40%)         43095                1%
  [40%,50%)         45196                1%
  [50%,60%)         56624                1%
  [60%,70%)         53013                1%
  [70%,80%)         65112                1%
  [80%,90%)         71479                2%
  [90%,100%)       100112                2%
  100%            1935854               57%
=================================================
-the number of debug variables processed: 3353169
-PC ranges covered: 56%
-------------------------------------------------
-total availability: 61%
=================================================

After:

 =================================================
           Debug Location Statistics
=================================================
    cov%           samples         percentage(~)
-------------------------------------------------
  0%               841874               25%
  (0%,10%)          41652                1%
  [10%,20%)         49847                1%
  [20%,30%)         49592                1%
  [30%,40%)         43230                1%
  [40%,50%)         45257                1%
  [50%,60%)         56594                1%
  [60%,70%)         52978                1%
  [70%,80%)         65097                1%
  [80%,90%)         71391                2%
  [90%,100%)        99888                2%
  100%            1935817               57%
=================================================
-the number of debug variables processed: 3353217
-PC ranges covered: 56%
-------------------------------------------------
-total availability: 61%
=================================================

(I've analyzed samples of both increases and decreases and confirmed that these changes are caused by my change in LCSSA and that the changes result in the correct behaviour)

Diff Detail

Event Timeline

n-omer created this revision.Dec 3 2020, 7:17 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 3 2020, 7:17 AM
n-omer requested review of this revision.Dec 3 2020, 7:17 AM
n-omer updated this revision to Diff 309255.Dec 3 2020, 7:35 AM

Added context to diff.

n-omer edited the summary of this revision. (Show Details)Dec 7 2020, 4:21 AM

LGTM, although couldn't you also just swap the basictest.ll DEBUGIFY-NEXT to be a DEBUGIFY-NOT: call void @llvm.dbg.value and not add the extra test.

(Nabeel and I share a corporate overloard, we should leave this a little more time in case anyone else wants to chip in).

LGTM, although couldn't you also just swap the basictest.ll DEBUGIFY-NEXT to be a DEBUGIFY-NOT: call void @llvm.dbg.value and not add the extra test.

(Nabeel and I share a corporate overloard, we should leave this a little more time in case anyone else wants to chip in).

Hi Jeremy, yes I could but in the interest of preserving context information about this change I thought that having a separate test is better. If you or anyone else feels strongly enough, I'm happy to make the change.

jmorse accepted this revision.Dec 16 2020, 6:28 AM

I wrote:

(Nabeel and I share a corporate overloard, we should leave this a little more time in case anyone else wants to chip in).

I'll land this on Nabeels behalf sometime soon.

This revision is now accepted and ready to land.Dec 16 2020, 6:28 AM
n-omer updated this revision to Diff 312214.Dec 16 2020, 8:01 AM

Transforms/LoopIdiom/X86/left-shift-until-bittest.ll test was recently added in this commit. The test in its current form relies on LCSSA's current behaviour. I have updated the patch to remove that reliance.

n-omer updated this revision to Diff 312493.Dec 17 2020, 7:25 AM

Resolve another merge conflict Transforms/LoopIdiom/X86/left-shift-until-bittest.ll.

This revision was automatically updated to reflect the committed changes.