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

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai added a comment.

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?



================
Comment at: lib/Target/PowerPC/PPCISelDAGToDAG.cpp:2979
     if (IsRHSZero)
       return getCompoundZeroComparisonInGPR(LHS, dl, ZeroCompare::LESExt);
+
----------------
echristo wrote:
> As a note I find the ZeroCompare enum to be fairly inscrutable. Perhaps document it and what each value is?
Will do.


Repository:
  rL LLVM

https://reviews.llvm.org/D38054