[PATCH, PowerPC] Fix invalid displacement created by LocalStackAlloc

[Hal Finkel]

----- Original Message -----
> From: "Ulrich Weigand" <Ulrich.Weigand at de.ibm.com>
> To: "llvm-commits" <llvm-commits at cs.uiuc.edu>
> Cc: "Hal Finkel" <hfinkel at anl.gov>
> Sent: Friday, July 11, 2014 8:08:57 AM
> Subject: [PATCH, PowerPC] Fix invalid displacement created by LocalStackAlloc
> 
> 
> 
> Hello,
> 
> the recent change to clang to enable dynamic stack reallocation for
> over-aligned byval parameters has triggered a pre-existing code-gen
> bug in
> LocalStackAlloc on PowerPC in one of my test cases.  (This has
> nothing to
> do with the dynamic realloc as such, the extra local copies simply
> enlarged
> the stack frame in the test case, so that stack displacements now
> start
> running out of range.)
> 
> The bug manifests in a MI instruction with out-of-range displacement
> after
> the LocalStackAlloc pass:
>         %vreg17<def> = LD 33184, %vreg31; mem:LD8[%g](align=32)
> G8RC:%vreg17 G8RC_and_G8RC_NOX0:%vreg31
> (In final assembler output the top bits are stripped off, resulting
> in a
> negative offset loading from below the stack pointer.)
> 
> This instruction was created by PPCRegisterInfo::resolveFrameIndex,
> which
> adds an offset to the FI offset already present in the instruction
> before,
> without verifying whether the resulting total offset is still in
> range.
> Now, the LocalStackAlloc common code will actually call another
> target
> routine, PPCRegisterInfo::isFrameOffsetLegal, to let the target
> decide
> whether the replacement is valid.  However, in the PowerPC
> implementation
> of that routine, we only check that whether the offset *delta* is
> itself in
> range, not whether resulting new *total* offset (FI offset already in
> the
> instruction *plus* the delta offset) is in range.
> 
> Common code however clearly expects the target to validate the total
> offset, and implementations of isFrameOffsetLegal on other targets
> agree
> with that, so it seems this is simply a bug in the PowerPC
> implementation.
> 
> Note that there is another call to isFrameOffsetLegal from *within*
> the
> PowerPC back-end (needsFrameBaseReg), where the *caller* already adds
> the
> instruction offset before calling isFrameOffsetLegal.   When changing
> isFrameOffsetLegal to perform this addition itself, that
> needsFrameBaseReg
> must then be adapted as well.
> 
> The following patch implements this, which fixes my test case (and
> passed
> regression test & bootstrap).
> 
> Is this OK to commit or am I missing something here?

Good catch! LGTM, thanks!

 -Hal

> 
> (See attached file: diff-llvm-fix-lsaoffset)
> 
> 
> Mit freundlichen Gruessen / Best Regards
> 
> Ulrich Weigand
> 
> --
>   Dr. Ulrich Weigand | Phone: +49-7031/16-3727
>   STSM, GNU/Linux compilers and toolchain
>   IBM Deutschland Research & Development GmbH
>   Vorsitzende des Aufsichtsrats: Martina Koederitz |
>   Geschäftsführung: Dirk
> Wittkopp
>   Sitz der Gesellschaft: Böblingen | Registergericht: Amtsgericht
> Stuttgart, HRB 243294

-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory