[PATCH] D97514: [SystemZ] Assign the full space for promoted and split outgoing args

[Ulrich Weigand via Phabricator via llvm-commits]

uweigand added a comment.

I'm not sure if "getTypeToTransformTo(*DAG.getContext(), OrigArgVT)" results in the same type that is used by common code in all cases.

If you look at code in TargetLowering::LowerCallTo and getCopyToParts, the logic seems to be different, and explicitly goes via a set of full "registers":

  MVT RegisterVT = getRegisterTypeForCallingConv(CLI.RetTy->getContext(),
                                                 CLI.CallConv, VT);
  unsigned NumRegs = getNumRegistersForCallingConv(CLI.RetTy->getContext(),
                                                   CLI.CallConv, VT);

and then

    if (NumParts * PartBits > ValueVT.getSizeInBits()) {
  [...]
        ValueVT = EVT::getIntegerVT(*DAG.getContext(), NumParts * PartBits);
        Val = DAG.getNode(ExtendKind, DL, ValueVT, Val);

so I'm wondering whether it wouldn't be better to follow that model here.  (E.g. by using CreateStackTemporary with a size instead of a type, and computing the size as "number-of-parts * size-of-part".)

Also, I think in any case it would be good to add an assertion when emitting the Store nodes that the target memory range of the store is actually within the allocated space, so that if we do get something wrong in the size calcuation the result will be an internal compiler error rather than wrong code.



================
Comment at: llvm/test/CodeGen/SystemZ/args-11.ll:13
+; CHECK-NEXT:    .cfi_offset %r14, -48
+; CHECK-NEXT:    .cfi_offset %r15, -40
+; CHECK-NEXT:    aghi %r15, -184
----------------
jonpa wrote:
> Not sure why 24 extra bytes are allocated and not 20... Alignment?
Yes, the stack always needs to be 8-byte aligned, and therefore all stack allocations are rounded up to the next multiple of 8.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97514/new/

https://reviews.llvm.org/D97514