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

[Kit Barton via llvm-commits]

kbarton added a comment.

Aside from minor comments this LGTM.
I think it would be good for @hfinkel  take a quick look too.



> PPCInstPrinter.cpp:474
> +
> +    /*  There are VSX instructions that use VSX register numbering (vs0 - vs63)
> +        as well as those that use VMX register numbering (v0 - v31 which

This should change to C++-style comments (i.e., //) as per the comment-formatting guidelines: http://llvm.org/docs/CodingStandards.html#comment-formatting

> PPCAsmPrinter.cpp:174
> +
> +    /*  There are VSX instructions that use VSX register numbering (vs0 - vs63)
> +        as well as those that use VMX register numbering (v0 - v31 which

Reformat comments

> PPCCallingConv.td:71
>    // ELFv2 ABI fully utilizes all these registers.
> -  CCIfType<[v16i8, v8i16, v4i32, v2i64, v1i128, v4f32], 
> +  CCIfType<[v16i8, v8i16, v4i32, v2f64, v2i64, v1i128, v4f32],
>             CCIfSubtarget<"hasAltivec()",

This is very minor, but this used to be sorted by integer types (in decending order of vector elements), followed by floats sorted in the same way. To maintain this, v2f64 should be moved to the end after v4f32 unless there is a specific reason it has been put where it is.

> PPCCallingConv.td:122
>             CCIfSubtarget<"hasQPX()", CCAssignToReg<[QF1, QF2]>>>,
> -  CCIfType<[v16i8, v8i16, v4i32, v2i64, v1i128, v4f32], 
> +  CCIfType<[v16i8, v8i16, v4i32, v2f64, v2i64, v1i128, v4f32],
>             CCIfSubtarget<"hasAltivec()",

Same comment about placement of v2f64.

> PPCCallingConv.td:192
>    // The first 12 Vector arguments are passed in AltiVec registers.
> -  CCIfType<[v16i8, v8i16, v4i32, v2i64, v1i128, v4f32], 
> +  CCIfType<[v16i8, v8i16, v4i32, v2f64, v2i64, v1i128, v4f32],
>             CCIfSubtarget<"hasAltivec()", CCAssignToReg<[V2, V3, V4, V5, V6, V7,

Same comment about v2f64

> PPCISelLowering.cpp:7079
>  static bool isNonConstSplatBV(BuildVectorSDNode *BVN, EVT Type) {
> +  if (BVN->isConstant())
> +    return false;

Could this condition be combined with the one below?

> PPCISelLowering.cpp:10577
> +
> +    SDValue FP;
> +    // For signed conversion, we need to sign-extend the value in the VSR

Is FP really needed? Can you just return on the lines where FP is being assigned?

> PPCISelLowering.cpp:10828
> +      } else {
> +        unsigned ByteWidth = N->getOperand(1).getValueType() == MVT::i8 ? 1 : 2;
> +        SDValue WidthConst = DAG.getIntPtrConstant(ByteWidth, dl, false);

ByteWidth might not be the best name here, since it shouldn't vary ;)

> PPCISelLowering.cpp:10829
> +        unsigned ByteWidth = N->getOperand(1).getValueType() == MVT::i8 ? 1 : 2;
> +        SDValue WidthConst = DAG.getIntPtrConstant(ByteWidth, dl, false);
> +

I don't see ByteWidth used anywhere else, so you can probably do away with it by just copying the logic to assign ByteWidth into this constructor call.

> PPCInstrInfo.cpp:1063
> +  if (Subtarget.hasVSX() && RC == &PPC::VRRCRegClass)
> +    RC = &PPC::VSRCRegClass;
> +

This is duplicated in both storeRegToStackSlot and loadRegFromStackSlot. 
This should be refactored into a separate method to get the TargetRegisterClass and called from both these methods instead, to ensure consistency.

> PPCMIPeephole.cpp:198
> +        bool SameOpcode = (MyOpcode == DefOpcode) ||
> +          (MyOpcode == PPC::VSPLTB && DefOpcode == PPC::VSPLTBs) ||
> +          (MyOpcode == PPC::VSPLTH && DefOpcode == PPC::VSPLTHs) ||

Is it possible for MyOpcode to be VSPLTBs also? I don't see this case handled anywhere and it's not clear that this situation could not be symmetric (i.e., MyOpcode == PPC::VSPLTBs and DefOpcode == PPC::VSPLTB).

> PPCRegisterInfo.td:286
>  
> -def VRRC : RegisterClass<"PPC", [v16i8,v8i16,v4i32,v2i64,v1i128,v4f32], 128,
> +def VRRC : RegisterClass<"PPC", [v16i8,v8i16,v4i32,v2i64,v2f64,v1i128,v4f32], 128,
>                           (add V2, V3, V4, V5, V0, V1, V6, V7, V8, V9, V10, V11,

Same comment about ordering.

Repository:
  rL LLVM

https://reviews.llvm.org/D23155