[PATCH] D38054: [PowerPC] Fix the spill issue exposed by r310809

[Eric Christopher via llvm-commits]

On Fri, Sep 22, 2017 at 7:19 AM Hal Finkel via Phabricator <
reviews at reviews.llvm.org> wrote:

> hfinkel added a comment.
>
> In https://reviews.llvm.org/D38054#878633, @nemanjai wrote:
>
> > In https://reviews.llvm.org/D38054#878445, @echristo wrote:
> >
> > > In https://reviews.llvm.org/D38054#877711, @hfinkel wrote:
> > >
> > > > I think this generally looks fine.
> > > >
> > > > Just as a general note on code organizations and commenting, we seem
> to be accumulating a pretty deep call tree here of functions that only work
> correctly in 64-bit mode. Not in this patch, but we should do something
> about this (either segregating these things into a distinct part of the
> file and/or adding asserts and/or comments to the 64-bit-mode-only
> functions.
> > >
> > >
> > > Agreed. Also a lot of the fine points of when to extend and when not
> could probably be split out as well. This is definitely getting pretty
> unwieldy.
> > >
> > > -eric
> >
> >
> > OK. I'll commit this patch as-is with the inline comment addressed.
> >
> > Then perhaps in a subsequent patch, I'll create something like `class
> GPRComparisonSelector` that uses essentially the same approach as the
> `BitPermutationSelector`. The `Select()` function will do the necessary
> checks and exit early if we're not a PPC64 target. All the functions that
> do the actual selection will be private. This way we should be able to
> avoid carelessly calling these functions under invalid conditions.
> >  Does that sound like a good plan?
>
>
> Yes, I think that would be better.
>
>
>
Agreed.

-eric



> Repository:
>   rL LLVM
>
> https://reviews.llvm.org/D38054
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170922/66c59d2c/attachment.html>