[PBQP] setEdgeCosts() (was PBQP bugfix)

Mon Feb 16 09:56:52 PST 2015

Hi Jonas,

Thanks for this!

Could you add some #ifndef NDEBUG guards too? We want to squeeze every byte
we can out of the Node and Edge Metadata structures in optimized builds, as
the graphs can get very large. Edges dominate, so your flag won't have a
big impact, but I still want to keep the overheads as low as possible.

Cheers,
Lang.

On Mon, Feb 16, 2015 at 7:44 AM, Jonas Paulsson <jonas.paulsson at ericsson.com
> wrote:

>  Hi Arnaud,
>
>
>
> Great, I committed the assert improvement as r229400.
>
>
>
> /Jonas
>
>
>
>
>
> *From:* Arnaud A. de Grandmaison [mailto:arnaud.degrandmaison at arm.com]
> *Sent:* den 16 februari 2015 11:11
>
> *To:* Jonas Paulsson; Lang Hames
> *Cc:* Commit Messages and Patches for LLVM
> *Subject:* RE: [PBQP] setEdgeCosts() (was PBQP bugfix)
>
>
>
> Hi Jonas,
>
>
>
> The assert is all about making sure we had choice when spilling a node ---
> but it does not attempt to question how good the choice is. I think your
> proposal to enhance the assert to all nodes which have gone thru the
> AllocatableNodes set is good to have.
>
>
>
> Regarding the second point, which is about what to do when several options
> for a node have the same minimal cost, the situation is a bit more
> complicated. The approach we have today is a bit too simple, but I do not
> think always preferring not to spill is a better approach. I think the
> right approach would be, in case of  ties, to ensure the selection for this
> node will not prevent a good selection in the neighbouring nodes, i.e. a
> local decision at the node should not prevent global cost improvement. This
> can be handled either at the reduction stage (improve reduction order), or
> at the backpropagate stage (do some look-ahead).  FYI, this is a problem we
> already have, without even speaking of spilling nodes.
>
>
>
> Cheers,
>
> --
>
> Arnaud
>
>
>
> *From:* Jonas Paulsson [mailto:jonas.paulsson at ericsson.com
> <jonas.paulsson at ericsson.com>]
> *Sent:* 16 February 2015 10:25
> *To:* Lang Hames; Arnaud De Grandmaison
> *Cc:* Commit Messages and Patches for LLVM
> *Subject:* RE: [PBQP] setEdgeCosts() (was PBQP bugfix)
>
>
>
> Hi Lang and Arnaud,
>
>
>
> What about the case where all cost elements are equal and non-infinite –
> would it then make sense to prefer a register allocation over spilling?
> Right now the node always gets spilled in that case and I am just curious
> if this is a decision that has been taken?
>
>
>
> /Jonas
>
>
>
>
>
>
>
> *From:* Lang Hames [mailto:lhames at gmail.com <lhames at gmail.com>]
> *Sent:* den 13 februari 2015 19:19
> *To:* Arnaud A. de Grandmaison
> *Cc:* Jonas Paulsson; Commit Messages and Patches for LLVM
> *Subject:* Re: [PBQP] setEdgeCosts() (was PBQP bugfix)
>
>
>
> Hi Jonas, Arnaud,
>
>
>
> That assert sounds good to me: If this node was conservatively allocatable
> then there must be some finite-cost register option available during
> back-propagation.
>
>
>
> Cheers,
>
> Lang.
>
>
>
> On Fri, Feb 13, 2015 at 4:27 AM, Arnaud A. de Grandmaison <
> arnaud.degrandmaison at arm.com> wrote:
>
> I removed the assert with r229103 (for now). The condition for the assert
> should indeed be that the node is spilled because there was no register
> available.  I still have to think where is the best place to have this
> assert, but it seems to me this should be in backpropagate. Something like
> (in pseudo code):
>
>
>
> If (G.getNodeMetaData(NId).isConservativelyAllocatable()) {
>
>   assert(v.getLength() > 1 && “node should have some register options”) ;
>
>   assert(!v[1 .. v.getLenght()-1].hasOnlyInfinities() && “A spillable only
> node is not conservatively allocatable”);
>
> }
>
>
>
> 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:* 13 February 2015 11:04
> *To:* 'Jonas Paulsson'; Lang Hames
>
>
> *Cc:* Commit Messages and Patches for LLVM
> *Subject:* RE: [PBQP] setEdgeCosts() (was PBQP bugfix)
>
>
>
> I’ll remove the assert until we can come up with a finer grained assert.
>
>
>
> Cheers,
>
> Arnaud
>
>
>
> *From:* Jonas Paulsson [mailto:jonas.paulsson at ericsson.com
> <jonas.paulsson at ericsson.com>]
> *Sent:* 13 February 2015 08:41
> *To:* Lang Hames
> *Cc:* Arnaud De Grandmaison; Commit Messages and Patches for LLVM
> *Subject:* RE: [PBQP] setEdgeCosts() (was PBQP bugfix)
>
>
>
> Hi,
>
>
>
> I’m thinking we could have an assert that a spilled conservatively node
> isn’t a “forced spill”. It isn’t wrong to choose the spill option per what
> you just wrote, but it is wrong if there are no registers available, right?
>
>
>
> /Jonas
>
>
>
>
>
> *From:* Lang Hames [mailto:lhames at gmail.com <lhames at gmail.com>]
> *Sent:* den 12 februari 2015 19:46
> *To:* Jonas Paulsson
> *Cc:* Arnaud A. de Grandmaison; Commit Messages and Patches for LLVM
> *Subject:* Re: [PBQP] setEdgeCosts() (was PBQP bugfix)
>
>
>
> Hi Jonas,
>
>
>
> You're right: Just because a node is conservatively allocatable doesn't
> mean it must be allocated a register. I hadn't thought this assert all the
> way through. ;)
>
>
>
> As an example of where spilling may be cheaper: imagine the solver has
> proved (via some chain of R0/RI/RII reductions) that allocating a register
> to this node will cause a more expensive node to be spilled. In that case
> it's better to spill this node and leave the register available for the
> node further down the stack.
>
>
>
> Arnaud - do you mind removing your assert?
>
>
>
> Cheers,
>
> Lang.
>
>
>
>
>
> On Wed, Feb 11, 2015 at 10:13 AM, Jonas Paulsson <
> jonas.paulsson at ericsson.com> wrote:
>
> Hi Arnaud,
>
>
>
> Your patch worked great for me, thank you.
>
>
>
> I however ran into a test case which triggered the assert. It turns out
> that the spill cost and all other options were equal and non-infinite.
> minIndex() then returned 0 - the spill-option, and so a conservatively
> allocable node got spilled even though it did not have to.
>
>
>
> I am now wondering if it could actually be that it is ok to spill a
> conservatively allocable node if that is cheaper than selecting a register
> option? Or perhaps it is an error to spill if a register is available
> during the back propagation?
>
>
>
> /Jonas
>
>
>
>
>
> *From:* Arnaud A. de Grandmaison [mailto:arnaud.degrandmaison at arm.com]
> *Sent:* den 11 februari 2015 10:13
> *To:* 'Lang Hames'; Jonas Paulsson
>
>
> *Cc:* Commit Messages and Patches for LLVM
> *Subject:* RE: [PBQP] setEdgeCosts() (was PBQP bugfix)
>
>
>
> Committed (with the change to isSpillable) @ r228816
>
>
>
> Jonas, you may want to give it a try and see if it fixes the issue you had
> with your target.
>
>
>
> Cheers,
>
> --
>
> Arnaud
>
>
>
> *From:* Lang Hames [mailto:lhames at gmail.com <lhames at gmail.com>]
> *Sent:* 10 February 2015 20:06
> *To:* Arnaud De Grandmaison
> *Cc:* Commit Messages and Patches for LLVM
> *Subject:* Re: [PBQP] setEdgeCosts() (was PBQP bugfix)
>
>
>
> 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/20150216/1097bbab/attachment.html>