[PATCH] PR18825 - New RegisterCoalescer subtarget hook

[Eric Christopher]

Haven't looked at the, but feel free to post-review them since the
idea is already ok and it should be a pretty direct translation.

-eric

On Wed, Jul 16, 2014 at 1:15 PM, Chris Bieneman <beanz at apple.com> wrote:
> Here are updated patches to move the hook from the subtarget to the
> TargetRegisterInfo.
>
> There are no functional differences from what is already committed.
>
> -Chris
>
>
> On Jul 16, 2014, at 10:29 AM, Eric Christopher <echristo at gmail.com> wrote:
>
> On Wed, Jul 16, 2014 at 10:28 AM, Jim Grosbach <grosbach at apple.com> wrote:
>
>
> On Jul 15, 2014, at 7:42 PM, Eric Christopher <echristo at gmail.com> wrote:
>
> On Tue, Jul 15, 2014 at 6:55 PM, Chris Bieneman <cbieneman at apple.com> wrote:
>
>
> On Jul 15, 2014, at 4:16 PM, Eric Christopher <echristo at gmail.com> wrote:
>
> Couple of small comments:
>
> +  unsigned SizeMultiplier = MBB->size()/100;
>
> Mmm.. random numbers. Why 100? Why not 101? How about a comment :)
>
>
> I can add a comment. The basic idea is that the heuristic restricts how many
> registers which meet certain criteria (large, sub-register indexed, …) get
> coalesced in a given basic block. It seems logical that the larger the basic
> block the more coalescing you should allow. Most of the tests in the
> llvm-test suite don’t hit this heuristic at all because they just don’t
> utilize many enough large registers. Unfortunately that means this really is
> a bit of a magical number.
>
>
> Yeah, it's nice to have it be some sort of documented constant rather
> than ... well, an undocumented constant. How'd you come up with the
> number btw?
>
>
>
> Otherwise... on the subtarget. Why there? Why not on the
> TargetRegisterInfo? Seems to make just as much sense for this use and
> TargetRegisterInfo already has a subtarget if you wanted to make cpu
> specific queries there.
>
> Thoughts?
>
>
> I hadn’t really considered putting it on the TargetRegisterInfo. When
> Quentin and I discussed the patch off-list we discussed it as a subtarget
> hook, so that was how I implemented it. In retrospect I still like it in the
> subtarget because the heuristic feels to me like it is a property of the
> subtarget, but it could be implemented in the TargetRegisterInfo.
>
>
> Conveniently TargetRegisterInfo is owned by each subtarget so it's
> like a subtarget hook :)
>
>
> Either seems fairly reasonable to me. I slightly prefer TargetRegisterInfo
> since the coalescer is already looking to that. So as long as that’s
> straightforward to implement, I’d go that direction.
>
>
> Agreed. Thanks.
>
> -eric
>
> -Jim
>
>
> I just figured since this seemed to be a register oriented type of
> hook that it might make sense to go there. It's one of those things
> that we haven't really done in the past, but that seemed to be because
> of the confusion with what a Subtarget was versus a Target.
>
> Quentin, do you have any thoughts on this?
>
>
> Jim generally has good insight here too. Jim?
>
> -eric
>
> -Chris
>
>
> -eric
>
>
>
> On Tue, Jul 15, 2014 at 7:31 AM, Quentin Colombet <qcolombet at apple.com>
> wrote:
>
> Hi Chris,
>
> Thanks for the update, I like what you did with the MachineFunctionInfo!
>
> LGTM with two comments:
> - AFAICT, we do not put anymore the date in filename for test cases.
> - For the out-of-registers.ll test case, please add some CHECK lines, just
> to be sure we are generating something correct.
>
> Thanks,
> -Quentin
>
> On Jul 14, 2014, at 10:26 PM, Chris Bieneman <cbieneman at apple.com> wrote:
>
> Quentin,
>
> Here are updated diffs with the changes you suggested. I’ve also included
> new test cases, one to test vector spilling, and one to test PR18825.
>
> If there are no other concerns about this patch I will commit it tomorrow.
>
> Thank you Quentin and David for the feedback.
>
> -Chris
> <18825.diff>
>
> On Jul 14, 2014, at 5:00 PM, Chris Bieneman <cbieneman at apple.com> wrote:
>
> Thanks for the review Quentin. It shouldn't be hard to move the DenseMap
> into the MachineFunctionInfo, I'll rework the patch, and I will provide test
> cases for both of the cases you mentioned.
>
> To close the loop on David's earlier question about the llvm performance
> suite. This patch has no impact on the generated code in any of our tests,
> so it will not alter performance.
>
> Thanks,
> -Chris
>
> On Jul 14, 2014, at 3:56 PM, Quentin Colombet <qcolombet at apple.com> wrote:
>
> Hi Chris,
>
> The current patch LGTM, though the DenseMap in the Subtarget is not pretty.
> My main concern is with big module, where the map will keep a lot of useless
> information since we have no way to know when it is fine to clear it.
>
> I was thinking moving that map into the XXXMachineFunctionInfo, not sure how
> feasible is that.
>
> Regarding the final patch, assuming no further feedbacks come, I would
> expect to have a new test case for PR18825 and the update test for
> CodeGen/ARM/vldm-sched-a9.ll.
>
> Thanks,
> -Quentin
>
> On Jul 14, 2014, at 1:23 PM, Chris Bieneman <cbieneman at apple.com> wrote:
>
> This morning I built SPEC with and without my patch and found no codegen
> differences.
>
> -Chris
>
> On Jul 11, 2014, at 5:19 PM, Chris Bieneman <cbieneman at apple.com> wrote:
>
> David,
>
> I agree completely. I'll look into any codegen differences in our test
> suite, and in SPEC.
>
> -Chris
>
>
>
> On Jul 11, 2014, at 4:26 PM, David Peixotto <dpeixott at codeaurora.org> wrote:
>
> Hi Chris,
>
> I agree that there is no simple solution. I am not opposed to your heuristic
> approach. I asked for stats or perf data because I know that the bug rarely
> happens, but I can't tell from reading the code how often the heuristic will
> hit. As you mentioned coalescing can have positive or negative performance
> impact and it would be good to understand if this change has any impact on
> codegen or performance for some larger benchmarks.
>
> That's my opinion anyway :)
>
> -David
>
> -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted
> by The Linux Foundation
>
> -----Original Message-----
> From: Chris Bieneman [mailto:cbieneman at apple.com]
> Sent: Friday, July 11, 2014 2:58 PM
> To: David Peixotto
> Cc: llvm-commits
> Subject: Re: [PATCH] PR18825 - New RegisterCoalescer subtarget hook
>
> Hi David,
>
> I haven’t run SPEC, but I have run all of LLVM’s in tree tests. Among
> those I haven’t found any codegen differences except the one test that I
> called out in my original email, and I suspect that would be a performance
> improvement.
>
> That said, there are certainly cases where blocking coalescing can
> generate worse code. I’ve been emailing back and forth with Quentin about
> potential longer-term solutions to this problem and there really is no
> simple solution.
>
> -Chris
>
> On Jul 11, 2014, at 2:45 PM, David Peixotto <dpeixott at codeaurora.org>
>
> wrote:
>
>
> Hi Chris,
>
> It would be great to get this bug fixed. I have a general question about
>
> the heuristic.
>
>
> I believe the case we need to avoid in the bug is quite rare (running
>
> out of registers), but the heuristic will prevent colaescing in other
> cases as well. Do you have any statistics for how many coalescings are
> prevented by the heuristic and any performance data for spec or llvm test
> suite?
>
>
> Thanks,
> David
>
> -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> hosted by The Linux Foundation
>
> -----Original Message-----
> From: llvm-commits-bounces at cs.uiuc.edu [mailto:llvm-commits-
> bounces at cs.uiuc.edu] On Behalf Of Chris Bieneman
> Sent: Friday, July 11, 2014 1:45 PM
> To: llvm-commits
> Subject: [PATCH] PR18825 - New RegisterCoalescer subtarget hook
>
> The attached patch adds a new subtarget hook to the
> RegisterCoalescer, and an ARM implementation. The ARM implementation
> works around PR18825, and also better code in cases with a large number
>
> of NEON vectors in use.
>
>
> The basic problem is that the coalescer is very aggressive at
> propagating constraints on the register classes, and the register
> allocator doesn’t know how to split sub-registers later to recover.
> This patch provides an escape valve for targets that encounter this
>
> problem to limit coalescing.
>
>
> A few other notes about this patch:
>
> (1) In code that triggers the heuristic (using a lot of large vector
> registers) this code can improve compile time substantially, and in
> other code it has no adverse impact.
>
> (2) This code also breaks one of the tests in our test suite
> (CodeGen/ARM/vldm-sched-a9.ll). This test is designed to test NEON
> vector spilling, and with this patch the test no longer spills. With
> the final patch I will provide a new test case to test vector
> spilling, and this test will be marked XFAIL so that we don’t regress
> this and start spilling here again.
>
> Thanks,
> -Chris
>
>
>
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
>