[PATCH] D23155: Power9 - Part-word VSX integer scalar loads/stores and sign extend instructions

[Hal Finkel via llvm-commits]

hfinkel added inline comments.


> PPCInstPrinter.cpp:479
> +        Namely, we print 32-63 when the instruction operates on one of the
> +        VMX registers. We previously had the VSHRC register class for the
> +        upper 32 VSX registers.

We don't need to mention how it used to work, that won't help anyone looking at the current code, and might be confusing.

> PPCAsmPrinter.cpp:179
> +        Namely, we print 32-63 when the instruction operates on one of the
> +        VMX registers. We previously had the VSHRC register class for the
> +        upper 32 VSX registers.

Same here; we don't need to mention how it used to work.

> PPCInstrInfo.cpp:1058
>  
> +  // Because VRRC and VSRC use incompatible store/load instruction, and backend
> +  // can spill VRRC to a frame then reload it by using VSRC, we have to make

We should make this comment more specific, and explain that the difference is that the lanes are stored in a different order.

> PPCInstrInfo.cpp:1061
> +  // sure this is not going to happen, so when target has VSX, we just
> +  // spill/reload VRRC by VSRC.
> +  if (Subtarget.hasVSX() && RC == &PPC::VRRCRegClass)

It would be better if we could explain here why this happens. It is not obvious because the live range that was split via the spill/restore would have had some particular register class.

> PPCInstrVSX.td:96
> +// operations rather than coalescing them into a single register. This simply
> +// forces the instruction to use the same register for both inputs.
> +class XX3Form_2s<bits<6> opcode, bits<5> xo, dag OOL, dag IOL, string asmstr,

I don't really understand what this is saying and why using the same register would matter.

> PPCInstrVSX.td:1125
>  let AddedComplexity = 400 in { // Prefer VSX patterns over non-VSX patterns.
> -  let isCommutable = 1 in {
> +  let isCommutable = 1, UseVSXReg = 1 in {
>      def XXLEQV : XX3Form<60, 186,

Why don't you set UseVSXReg = 1 in an outer block (essentially over all definitions), and then override by setting UseVSXReg = 0 for those instructions that shouldn't have it. I'm under the impression that this is a smaller set.

Repository:
  rL LLVM

https://reviews.llvm.org/D23155