[PBQP] setEdgeCosts() (was PBQP bugfix)

Tue Feb 10 11:06:21 PST 2015

Hi Arnaud,

This looks good to me, with one small change: Since OptimallyReducible
nodes can still be spilled, isSpillable should be:

bool isSpillable() const {
return RS == NotProvablyAllocatable || RS == OptimallyReducible; }

Cheers,
Lang.

On Tue, Feb 10, 2015 at 9:12 AM, Arnaud A. de Grandmaison <
arnaud.degrandmaison at arm.com> wrote:

> This is an updated version of the patch, where an assertion now enforces
> that a node never gets downgraded.
>
>
>
> I think I can commit it as there are now assertions checking all
> invariants (only NotProvablyAllocatable nodes can be spilled + a node’s
> ReductionState cannot be downgraded).
>
>
>
> Cheers,
>
> --
>
> Arnaud
>
>
>
> *From:* llvm-commits-bounces at cs.uiuc.edu [mailto:
> llvm-commits-bounces at cs.uiuc.edu] *On Behalf Of *Arnaud A. de Grandmaison
> *Sent:* 09 February 2015 23:44
> *To:* 'Lang Hames'
> *Cc:* Commit Messages and Patches for LLVM
> *Subject:* RE: [PBQP] setEdgeCosts() (was PBQP bugfix)
>
>
>
> Given that nodes can never go back, why not just checking the NodeMetadata
> as in the attached updated patch (see NodeMetadata::isSpillable()): only
> nodes from the NotProvablyAllocatable set can be spilled. Anyother case
> from the ReductionState enum is an error.
>
>
>
> Now, my sentence above starts with ‘given’… Yet another assertion to check
> J. Or not, as this is how the algorithm is written, and checking this is
> a bit of an overkill (I think). It could however be worth adding a comment
> to promote / moveToConservativelyAllocatableNodes &
> moveToOptimallyReducibleNodes to document this assumption.
>
>
>
> Cheers,
>
> --
>
> Arnaud
>
>
>
>
>
> *From:* Lang Hames [mailto:lhames at gmail.com <lhames at gmail.com>]
> *Sent:* 09 February 2015 19:47
> *To:* Arnaud De Grandmaison
> *Cc:* Jonas Paulsson; Commit Messages and Patches for LLVM
> *Subject:* Re: [PBQP] setEdgeCosts() (was PBQP bugfix)
>
>
>
> Hi Arnaud,
>
>
>
> About the assert - sorry I haven't applied that yet. When I was thinking
> of shuffling nodes back and forward between sets it seemed expensive: The
> verification set would have to be updated too, which would be expensive and
> not particularly helpful for readability. That's no longer an issue (since
> your patch never moves things back to less-allocatable sets), but I think
> we can still do better with the assert: We can expand the reduction stack
> to hold a NodeID plus an enum that tells us how the node was reduced (i.e.
> this was pushed to the stack from the conservatively-allocatable set).
> Then, when we rebuild the graph and assign options we can assert that
> conservatively-allocatable nodes are never spilled. How does that sound?
>
>
>
> Cheers,
>
> Lang.
>
>
>
> On Mon, Feb 9, 2015 at 10:07 AM, Arnaud A. de Grandmaison <
> arnaud.degrandmaison at arm.com> wrote:
>
> I had nothing specific in mind, I am just not ruling out what can happen
> if the reduction rules get changed.
>
>
>
> I think it is good to check that no spilled node comes from the
> OptimalyReducible or ConservativelyAllocatable sets though, as this would
> indicate something failed.
>
>
>
> I’ll update my patch later today.
>
>
>
> Cheers,
>
> --
>
> Arnaud
>
>
>
> *From:* Lang Hames [mailto:lhames at gmail.com]
> *Sent:* 09 February 2015 18:28
>
>
> *To:* Arnaud De Grandmaison
> *Cc:* Jonas Paulsson; Commit Messages and Patches for LLVM
> *Subject:* Re: [PBQP] setEdgeCosts() (was PBQP bugfix)
>
>
>
> 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 <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 <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/20150210/0403b641/attachment.html>