[llvm-commits] [PATCH, PowerPC] Fix PR13949

[Hal Finkel]

----- Original Message -----
> From: "Bill Schmidt" <wschmidt at linux.vnet.ibm.com>
> To: llvm-commits at cs.uiuc.edu
> Sent: Monday, October 15, 2012 10:04:35 PM
> Subject: [llvm-commits] [PATCH, PowerPC] Fix PR13949
> 
> This patch addresses PR13949.
> 
> For the PowerPC 64-bit ELF Linux ABI, aggregates of size less than 8
> bytes are to be passed in the low-order bits ("right-adjusted") of
> the
> doubleword register or memory slot assigned to them.  A previous
> patch
> addressed this for aggregates passed in registers.  However, small
> aggregates passed in the overflow portion of the parameter save area
> are
> still being passed left-adjusted.
> 
> The fix is made in PPCTargetLowering::LowerCall_Darwin_Or_64SVR4 on
> the
> caller side, and in PPCTargetLowering::LowerFormalArguments_64SVR4 on
> the callee side.  The main fix on the callee side simply extends
> existing logic for 1- and 2-byte objects to 1- through 7-byte
> objects,
> and correcting a constant left over from 32-bit code.  There is also
> a
> fix to a bogus calculation of the offset to the following argument in
> the parameter save area.
> 
> On the caller side, again a constant left over from 32-bit code is
> fixed.  Additionally, some code for 1, 2, and 4-byte objects is
> duplicated to handle the 3, 5, 6, and 7-byte objects for SVR4 only.
>  The
> LowerCall_Darwin_Or_64SVR4 logic is getting fairly convoluted trying
> to
> handle both ABIs, and I propose to separate this into two functions
> in a
> future patch, at which time the duplication can be removed.
> 
> The patch adds a new test (structsinmem.ll) to demonstrate correct
> passing of structures of all seven sizes.  Eight dummy parameters are
> used to force these structures to be in the overflow portion of the
> parameter save area.
> 
> As a side effect, this corrects the case when aggregates passed in
> registers are saved into the first eight doublewords of the parameter
> save area:  Previously they were stored left-justified, and now are
> properly stored right-justified.  This requires changing the expected
> output of existing test case structsinregs.ll.
> 
> The patch has been tested on powerpc64-unknown-linux-gnu with no new
> regressions.  Is this ok to commit?

A few comments:

> +; Note that the code generation for packed structs is very poor because
> +; the PowerPC target wrongly rejects all unaligned loads.  This test case
> +; will need to be revised when that is fixed.

This should be labeled 'FIXME'.

> +        // This must go outside the CALLSEQ_START..END.
> +        SDValue NewCallSeqStart = DAG.getCALLSEQ_START(MemcpyCall,
> +                                    CallSeqStart.getNode()->getOperand(1));

Can you better explain this comment. Does 'This' refer to the MemcpyCall?

Otherwise, LGTM.

Thanks again,
Hal

> 
> Thanks,
> Bill
> --
> Bill Schmidt, Ph.D.
> IBM Advance Toolchain for PowerLinux
> IBM Linux Technology Center
> wschmidt at us.ibm.com
> wschmidt at linux.vnet.ibm.com
> 
> 
> 
> 
> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> 

-- 
Hal Finkel
Postdoctoral Appointee
Leadership Computing Facility
Argonne National Laboratory