Re: [PATCH/MIPS] Fix Branches with a constant offset

[Andrew Pinski <andrew dot pinski at caviumnetworks dot com>]

On Wed, Jun 6, 2012 at 5:35 PM, Maciej W. Rozycki
<macro@codesourcery.com> wrote:
> Richard,
>
> [Removed David from the cc list as he's not been with MIPS Technologies
> for many years now and is not available to comment anymore.]
>
> On Wed, 6 Jun 2012, Richard Sandiford wrote:
>
>> > Â After:
>> > 2011-04-20 ÂCatherine Moore Â<clm@codesourcery.com>
>> > Â Â Â Â Â Â David Ung <davidu@mips.com>..
>> > Â Â Â Â * config/mips.c (mips_cl_insn): Add new field complete_p
>> > ...
>> > Â Â Â Â (append_insn): Move O_constant expression handling.
>> >
>> > branches with a constant expression are broken by adding a relocation to them.
>> > The relocation is totally wrong as it is saying it is an absolute
>> > address while what we have really is an offset.
>> >
>> > This patch fixes the issue with how complete_p is handled for
>> > O_constant expression handling just like it was handled before this
>> > patch.
>> >
>> > OK? ÂTested on mips64-linux-gnu with no regressions.
>>
>> Oops. ÂIt's certainly unfortunate that we've changed the meaning
>> of this case. ÂTBH though, the new version makes more sense to me.
>> If you really want a constant offset, ". + X" is (and IMO always was)
>> the right way to write it. ÂThe old behaviour gave oddities where:
>>
>>    .equ  Âx,0x1000
>>     beq   $4,$5,x
>>
>> would treat "x" as an offset while:
>>
>>     beq   $4,$5,x
>>    .equ  Âx,0x1000
>>
>> would treat it as an address. ÂThe new behaviour also seems to be
>> consistent with other targets.
>>
>> None of which would be a convincing argument to change the behaviour
>> for its own sake. ÂBut now that we've changed it anyway -- in at least
>> two releases -- I'm not sure we should go back.
>
> ÂFWIW, I think semantics should be:
>
> 1.
>    Âbeq   $4, $5, 0x1000
>
> Â -- branch to the absolute address 0x1000. ÂThat needs a relocation of
> Â course and may overflow.
>
> 2.
>
>    Âbeq   $4, $5, . + 0x1000
>
> Â -- branch to the relative address 0x1000 bytes away up from the address
> Â of the branch instruction. ÂNo relocation needed.
>
> That, of course, however the third operand is evaluated to its final
> expression. ÂAnything else is going to be pure confusion. ÂMy observation
> is this is what GAS currently does. ÂThis is also how J interprets its
> argument (except of course a relocation is also needed in the second case;
> I have no idea why one isn't produced to catch overflows in the first case
> though -- this is probably a bug).
>
> ÂI could envisage constructs like:
>
>    Âbeq   $4, $5, -0x1000
>
> and:
>
>    Âbeq   $4, $5, +0x1000
>
> where "-" and "+" are not the usual arithmetic unary operators and that
> would express relative offsets just as with #2 above, but that IMHO would
> only make sense where a new assembly syntax was being defined and in our
> case would also cause more confusion than possible gain.
>
> ÂAndrew, is the current implementation problematic for you for any reason?

Just the MIPS AVP testsuite for verify the architecture depends on the
older (incorrect also in my mind also) behavior.  I don't know who to
talk to at MIPS have this fixed in that testsuite.

Thanks,
Andrew Pinski