[PBQP] setEdgeCosts() (was PBQP bugfix)

Mon Feb 9 09:28:14 PST 2015

Hi Arnaud,

I'm not sure which other effects you had in mind, but in the back of my mind I was still thinking about mutable PBQP graphs. If we had those then we'd need to move nodes back out of the optimally-reducible/conservatively-allocatable sets when spill code is inserted. Since we don't have mutable graphs though, I think what you're doing seems safe. I'll run it over the test-suite on X86 when I get to the office.

Cheers,
Lang.

Sent from my iPad

> On Feb 9, 2015, at 8:14 AM, Arnaud A. de Grandmaison <arnaud.degrandmaison at arm.com> wrote:
> 
> Will do.
>  
> My patch only prevents “transient” effects, but does not catch other cases, which should arguably not happen.
>  
> Cheers,
> Arnaud
>  
> From: Lang Hames [mailto:lhames at gmail.com] 
> Sent: 09 February 2015 17:00
> To: Arnaud De Grandmaison
> Cc: Jonas Paulsson; Commit Messages and Patches for LLVM
> Subject: Re: [PBQP] setEdgeCosts() (was PBQP bugfix)
>  
> Hi Arnaud,
>  
> I was about to teach handleReconnectEdge to move things back NotConservativelyAllocatable (and from OptimallyReducible back up too). Your solution is much nicer.
>  
> Please commit away.
>  
> - Lang.
>  
> On Mon, Feb 9, 2015 at 4:58 AM, Arnaud A. de Grandmaison <arnaud.degrandmaison at arm.com> wrote:
> Hi Jonas,
>  
> I gave a look at this over the week-end, and I believe the attached patch can fix the issue you described.
>  
> The patch modifies handleSetCosts so that it only promotes the nodes to the OptimallyReducible or ConservayivelyAllocatable sets once the metadata have been fully updated. And while there, I renamed handleSetCosts to handleUpdateCosts to better reflect what the method does.
>  
> Cheers,
> --
> Arnaud
>  
> From: Jonas Paulsson [mailto:jonas.paulsson at ericsson.com] 
> Sent: 06 February 2015 12:22
> To: Lang Hames; Arnaud De Grandmaison
> Cc: llvm-commits at cs.uiuc.edu
> Subject: [PBQP] setEdgeCosts() (was PBQP bugfix)
>  
> Hi Lang,
>  
> The patch seems good to me on my target as well, thank you.
>  
> However, the bug#3 I wrote about earlier is now still merely hidden. The problem is that setEdgeCosts() temporarily disconnects two nodes, moves them to ConservativelyAllocatableNodes because their DeniedOpts have now been decreased by one, and then reconnects them. DeniedOpts gets increased again during reconnection, and the isConservativelyAllocatable() test would then fail, but the nodes have already been moved.
>  
> I resend the test case again along with reverts of patches that hide the bug (for the test case).
>  
> To investigate:
>  
> Revert the two patches that hide the bug (see below), or just apply the attached patches that do the same thing. (To see that there is an error, also apply the patch with an assert that no nodes in coservatively allocatables gets the spill option).
>  
> Then run llc with the attached test case:
>  
> llc pbqp_reduced.ll -mtriple=aarch64-none-linux-gnu -mcpu=cortex-a57 -mattr=+neon -optimize-regalloc -regalloc=pbqp
>  
> (The assertion triggers, because Node 2 was in conservatively allocatables and yet spilled.)
>  
> The error is made *during applyR2() of node 18*.
>  
> Debug dumps (patch for debug dumps not provided, sent earlier):
>  
> handleDisconnectEdge(9, 2) : DeniedOpts 10 -> 9
> NId 9(%vreg15, GPR64common)  moved to conservatively-allocatables.
> handleDisconnectEdge(2, 9) : DeniedOpts 10 -> 9
> NId 2(%vreg4, GPR64common)  moved to conservatively-allocatables.
>  
> Popped NId 2(%vreg4, GPR64common) , all edge costs added:
> 2.002748e+01 inf inf inf inf inf inf inf inf inf inf ** selection: 0
>  
> /Jonas
>  
> Attachments:
>  
> commit 4a44259e80c04c340349ed8f32bcbc8ffdb89b52
> Author: Jonas Paulsson <jonas.paulsson at ericsson.com>
> Date:   Fri Feb 6 10:34:35 2015 +0100
>  
>     Revert "[PBQP Regalloc] Pre-spill vregs that have no legal physregs."
>    
>     This reverts commit d54450ef63998effdb19476c4e4c6c3f0a8c5f50.
>  
> commit 9ada754d86920eba6cbda342e643e235788d139b
> Author: Jonas Paulsson <jonas.paulsson at ericsson.com>
> Date:   Fri Feb 6 10:30:38 2015 +0100
>  
>     Revert "[PBQP] Fix transposed worst row/column check in handleAdd/RemoveNode in the PBQP"
>    
>     This reverts commit 4bde7909b4eb2aba2cd62b79d8cca98d9af6692e.
>  
> commit 45b838cc499a6af384c43fbc2d8c83c17b48f25e
> Author: Jonas Paulsson <jonas.paulsson at ericsson.com>
> Date:   Wed Jan 28 08:30:35 2015 +0100
>  
>     Assert in PBQP that a node that selects '0' (spilled) was not pushed on node
>     stack as conservatively allocatable.
>  
> Test case: pbqp_reduced.ll
>  
>  
>  
> From: Lang Hames [mailto:lhames at gmail.com] 
> Sent: den 3 februari 2015 07:19
> To: Jonas Paulsson; Arnaud A. de Grandmaison
> Cc: llvm-commits at cs.uiuc.edu
> Subject: Re: PBQP bugfix
>  
> Hi Jonas,
>  
> Apologies for the delay - I've committed my fix for this in r227942.
>  
> I haven't had a chance to try it out on your ARM test case yet, but hope to soon. Please let me know if it doesn't fix the problem for your out-of-tree target.
>  
> Cheers,
> Lang.
>  
>  
> On Fri, Jan 30, 2015 at 11:10 AM, Lang Hames <lhames at gmail.com> wrote:
> Hi Jonas,
>  
> Thanks very much for tracking this down!
>  
> I think the best approach to fixing this is to have the PBQP register allocator recognize these nodes early and never represent them in the graph. Any matrix that has only one column must be connected to a node that has only one solution, and this should be allocated up-front to simplify the solver's job.
>  
> I'll have a fix for this later today.
>  
> Cheers,
> Lang.
>  
> On Thu, Jan 29, 2015 at 9:25 AM, Jonas Paulsson <jonas.paulsson at ericsson.com> wrote:
> Hi,
>  
> This is a bugfix for the PBQP register allocator. The case for a one-column matrix must be handled in the MatrixMetadata constructor.
>  
> See commit message for further explanation,
>  
> Jonas Paulsson
>  
>  
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> 
>  
>  
>  
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150209/581ff6b2/attachment.html>