[PATCH] Add experimental PBQP support

[Lang Hames]

Hi Arnaud,

I think you're right. Please go ahead and commit your current patch
whenever you're ready. We can make the allocator-configuration patch
afterwards.

Cheers,
Lang.

On Mon, Sep 8, 2014 at 11:47 PM, Arnaud A. de Grandmaison <
arnaud.degrandmaison at arm.com> wrote:

> Yes, that would work. I can prepare a patch for that – unless you want to
> do it.
>
>
>
> Do you feel we should commit this before or after my patch is committed ?
> I think it makes sense to commit it after, as it would be used right away
> by the aarch64 backend, without having to wait for a subsequent commit.
>
>
>
> *From:* Lang Hames [mailto:lhames at gmail.com]
> *Sent:* 08 September 2014 21:40
> *To:* Arnaud De Grandmaison
>
> *Subject:* Re: [PATCH] Add experimental PBQP support
>
>
>
> Hi Arnaud,
>
>
>
> My (vague) idea is to add a TargetSubtargetInfo parameter to the register
> allocator create.* functions. Most of the register allocators would ignore
> this argument, but PBQP's create would then look like:
>
>
>
> FunctionPass* llvm::createDefaultPBQPRegisterAllocator(const
> TargetSubtargetInfo &STI) {
>   return createPBQPRegisterAllocator(STI.createPBQPRAConstraintsBuilder());
> }
>
>
>
> The default implementation of createPBQPRAConstraintsBuilder() would just
> return the standard interference builder. Targets could override this to
> return something different.
>
>
>
> I believe that would preserve all the use cases you described above, and
> avoid the hacks to the pass setup.
>
>
>
> - Lang.
>
>
>
> On Mon, Sep 8, 2014 at 12:46 PM, Arnaud A. de Grandmaison <
> arnaud.degrandmaison at arm.com> wrote:
>
>
>
>
>
> *From:* Lang Hames [mailto:lhames at gmail.com]
> *Sent:* 08 September 2014 20:06
> *To:* Arnaud De Grandmaison
> *Cc:* David Blaikie; Tim Northover; Commit Messages and Patches for LLVM
> *Subject:* Re: [PATCH] Add experimental PBQP support
>
>
>
> Hi Arnaud,
>
>
>
> One aspect of this that Dave drew my attention to is the -aarch64-pbqp
> flag. I might look at tweaking the way register allocators are created so
> that the Target can supply a PBQP problem builder. That way the
> '-regalloc=pbqp' would just work. Then you could avoid the hacks in the
> pass manager setup.
>
>
>
> I also thought about that, but without having a testcase showing it, it
> would have been difficult to  upstream any change.
>
>
>
> At least, what we have today is flexible enough that:
>
> - targets that need no specific PBQPBuilder just use the –regalloc=pbqp
>
> - targets can subclass the PBQPBuilder, while still using the generic
> build method, like aarch64 does
>
> - targets can completely override the generic build method --- and not
> even use it
>
>
>
> I think it is important to keep those 3 possibilities, so that people who
> want to experiment can do it easily.
>
>
>
> I have not been able to come up with a good solution, or at least one
> which is worth the changes implied. If you come up with a better solution,
> I will gladly use it J
>
>
>
> If I come up with a good solution I'll let you know.
>
>
>
> -Lang.
>
>
>
>
>
> On Mon, Sep 8, 2014 at 11:05 AM, Lang Hames <lhames at gmail.com> wrote:
>
> Hi Arnaud,
>
>
>
> The PBQP side side of this looks good to me. Thanks very much for working
> on this.
>
>
>
> Regards,
>
> Lang.
>
>
>
> On Mon, Sep 8, 2014 at 7:32 AM, Arnaud A. de Grandmaison <
> arnaud.degrandmaison at arm.com> wrote:
>
>
>
>
>
> *From:* David Blaikie [mailto:dblaikie at gmail.com]
> *Sent:* 08 September 2014 04:53
> *To:* Arnaud De Grandmaison
> *Cc:* Tim Northover; llvm-commits at cs.uiuc.edu; Lang Hames
> *Subject:* Re: [PATCH] Add experimental PBQP support
>
>
>
>
>
>
>
> On Sat, Sep 6, 2014 at 2:01 PM, Arnaud A. de Grandmaison <
> arnaud.degrandmaison at arm.com> wrote:
>
>
>
>
>
> *From:* David Blaikie [mailto:dblaikie at gmail.com]
> *Sent:* 06 September 2014 21:40
> *To:* Arnaud De Grandmaison
> *Cc:* Tim Northover; llvm-commits at cs.uiuc.edu; Lang Hames
> *Subject:* Re: [PATCH] Add experimental PBQP support
>
>
>
>
>
>
>
> On Sat, Sep 6, 2014 at 8:30 AM, Arnaud A. de Grandmaison <
> arnaud.degrandmaison at arm.com> wrote:
>
> Writing tests for a register allocator is not an easy task, as the set of
> all valid allocation is quite large, and can be equally good. What I have
> seen with the other allocators is that most testcases correspond to
> specific issues found in the allocator. My plan was to have an initial
> commit (this patch, with no real test), and then add testcases with
> subsequent commits as they improve specific areas of the allocation.
>
>
>
> so this change itself doesn't add any improvements, just lays the
> foundation for improvements to come?
>
>
>
>
>
> Correct. I see this patch as a foundation for improvements to come.
>
>
>
> Great - perhaps you could commit the small test case you've added here
> ahead of time (to demonstrate that it passes without these changes), just
> adding more test coverage.
>
>
>
>
>
> The test is merely a sanity check that the’ –aarch64-pbqp’ option exists
> and produce something sane. A testcase committed ahead of time would be a
> duplicate of r217057, a sanity check Lang added some time ago.
>
>
>
>  This patch only uses the existing infrastructure as is,  and was
> necessary to run a wide range of benchmarks and diagnose where improvements
> should be made.
>
>
>
> Not sure I follow here - according to you & Lang the PBQP allocator
> already works on these architectures. How does this patch help you diagnose
> where improvements are to be made?
>
> The PBQP works in the sense that the generate code is functionally
> correct, i.e there is no miscompilation --- no bogus register allocation.
> That’s a required preliminary before any performance improvement can take
> place. With this patch, we have everything in place to be able to compare 2
> feature-wise similar versions of llvm for the AArch64/A57:
>
> - LLVM with greedy + A57 FPLoadBalancing pass
>
> - LLVM with PBQP + A57 target specific constraints
>
> Both provide the same high level features, but with different
> implementations.
>
>
> That said, if it's a natural precursor/lays the foundation for the ability
> to add more custom logic to the PBQP allocator for these architectures -
> great, let's go for it! (though I'd suggest you wait for Lang's OK here -
> I'm not nearly familiar enough with this stuff, unfortunately - just trying
> to understand the high level nature of the change you're proposing, because
> i'm curious)
>
>
>
> It lays the foundations for the real work to take place.
>
>
>
> I will definitely wait for Lang’s OK for the PBQP aspects of the patch,
> and Tim’s green light for the AArch64 aspects !
>
>
>
>
>
> I already have a few on my list, the first one being improving how the
> different costs are set and relate together (allocation cost, interference
> cost & spill weight) --- and this will require some modification in the
> generic infrastructure and a backend with extra constraints to see the
> effects.
>
>
>
>
>
>
>
>
>
> *From:* David Blaikie [mailto:dblaikie at gmail.com]
> *Sent:* 06 September 2014 17:17
> *To:* Arnaud De Grandmaison
> *Cc:* Tim Northover; llvm-commits at cs.uiuc.edu; Lang Hames
> *Subject:* RE: [PATCH] Add experimental PBQP support
>
>
>
>
> On Sep 6, 2014 8:08 AM, "Arnaud A. de Grandmaison" <
> arnaud.degrandmaison at arm.com> wrote:
> >
> > Hi Dave & Lang,
> >
> >
> >
> > The AArch64 does not require extra constraints for the PBQP to work, but
> the AArch64/A57 benefits from setting additional constraints. On the A57,
> some sequence of operations will execute faster if some of their operands
> stays in even or odd registers. The Arch64FPLoadBalancing pass has been
> added to do some optimization there by permuting registers in the straight
> forward cases, whereas this can be solved generally and elegantly with the
> PBQP at register allocation time.
>
> Awesome - thanks for the explanation.
>
> Are the improvements separable into patches per specific improvement (with
> corresponding tests for each)?
>
> >
> >
> >
> > Cheers,
> >
> > Arnaud
> >
> >
> >
> > From: Lang Hames [mailto:lhames at gmail.com]
> > Sent: 06 September 2014 06:14
> > To: David Blaikie
> > Cc: Arnaud De Grandmaison; llvm-commits at cs.uiuc.edu; Tim Northover
> > Subject: Re: [PATCH] Add experimental PBQP support
> >
> >
> >
> > Hi Dave,
> >
> >
> >
> > Out-of-the-box PBQP knows about the standard constraints that CodeGen
> models. Any Target that works with the standard allocators (E.g. greedy)
> should also work with PBQP. I believe Arnaud's patch is an optimisation.
> (Arnaud - please correct me if I'm wrong and AArch64 did require extra
> constraints, but I don't think it should?)
> >
> >
> >
> > - Lang.
> >
> >
> >
> > On Fri, Sep 5, 2014 at 3:45 PM, David Blaikie <dblaikie at gmail.com>
> wrote:
> >
> > This'll probably show how little I know about register allocation - but
> I thought Lang was telling me the other day that PBQP is essentially a
> drop/opt in for any architecture without having specific code for it
> (learning about the register set from the tablegen files and that was all
> it needed).
> >
> > Is that the case? Is the extra code in your patch then tuning,
> essentially - making PBQP better than the baseline table-driven PBQP for
> AArch64/A57? Or is my understanding incorrect?
> >
> > - David
> >
> >
> >
> > On Fri, Sep 5, 2014 at 1:49 PM, Arnaud A. de Grandmaison <
> arnaud.degrandmaison at arm.com> wrote:
> >>
> >> I am currently investigating the benefits the PBQP register allocator
> could bring to the AArch64/A57.
> >>
> >>
> >>
> >> This patch adds experimental support for PBQP. The PBQP is disabled by
> default, and can be enabled with the ‘–aarch64-pbqp’ command line option to
> llc when the cortex-a57 is in use.
> >>
> >>
> >>
> >> I thought it would be a good thing to upstream this patch, as some
> other people in the community could be interested in experimenting with
> this allocator.
> >>
> >>
> >>
> >> It passes all the tests (LNT, spec, …), but the performance of the
> generated code is not optimal yet. Expect some more patches in the coming
> days to improve the performance.
> >>
> >>
> >>
> >> Cheers,
> >>
> >> --
> >>
> >> Arnaud A. de Grandmaison
> >>
> >>
> >>
> >>
> >>
> >> _______________________________________________
> >> 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/20140909/b9520bda/attachment.html>