[RFA/ARM] Fix PR54974: Thumb literal pools don't handle PC rounding

[Ramana Radhakrishnan]

On Wed, Nov 21, 2012 at 7:59 PM, Matthew Gretton-Dann
<matthew.gretton-dann@linaro.org> wrote:
> All,
>
> The attached patch fixes PR54974.
>
> In Thumb when calculating the PC value for a literal load the value used is
> the current PC rounded down to the nearest multiple of 4.  The ARM backend
> currently does not take this into account when calculating literal pool
> placement.
>
> The fix is to decrease the pool_range of all insns by 2 when generating
> Thumb code.  There is no need to change neg_pool_range values as rounding
> down here will reduce the distance of the literal pool.

A comment about this fact around thumb2_pool_range would be appropriate.

>
> The patch attached to the PR is not sufficient as we don't precisely know
> the PC when calculating literal pool ranges and so have to be conservative.

Correct, the PC value is never known in T32 state because you do not
know the lengths of all instructions accurately and you can never know
without additional calculations. Additionally your function can start
at a 2 byte alignment in certain cases IIRC so that adds more fuzz
.... :)



> Whilst going through all the code I found the following, possibly related,
> issues that I would like some input from the ARM maintainers on (although
> they have not been touched in this patch):
>
> 1) Some Thumb-2 patterns (like thumb2_movhi_insn) have a neg_pool_range of
> 250 for ldrh, where my reading of the ARMARM says the range is [-4095, 4095]
> for Thumb-2 (with appropriate rounding).  What is the reason for GCC's
> severe pessimism here?
>

That looks like a typo right from day 1 when this code seems to have
made it into trunk from 2007. I can see no reason for it to have the
value -250 today and I would treat a patch that fixes this up as
obvious for trunk but not for any release branches.


> 2) thumb1_zero_extendqisi2 (and other insns) give a Thumb-1 narrow ldrb a
> pool_range of 32.  Surely the pool_range should be 0 (or *) as Thumb-1
> doesn't have a ldrb where the base-register can be PC?


Another typo. Remove that - it shouldn't in theory have any effect as
we allow only *an* m constraint :) No `i' there so there is no chance
of an invalid constant appearing there. In addition an 8 bit move into
a register is a valid immediate move for thumb1 IIRC, so this should
all be ok . Deleting the pool range pattern from the pattern is ok .
This is again something that's ok only for trunk.


>
> Tested arm-none-linux-gnueabi cross, and with the testcase attached to the
> PR.  No added testcase in the patch as this code is sensitive to other code
> generation and so it is not easy to generate a testcase which will reliably
> test this condition.
>
> OK for trunk, 4.7, and 4.6?


Ok for trunk today - please wait a few days before backporting into
4.6 and 4.7 to see what the fallout is like . Watch out for any
fallout with the auto-testers.

regards
Ramana