Re: RFC: MIPS: Workaround the "daddi" and "daddiu" errata

[Richard Sandiford <rsandifo at redhat dot com>]

"Maciej W. Rozycki" <macro@ds2.pg.gda.pl> writes:
>> Of course ((void (*)(void))0xffffffffbfc00000) is a valid object.
>> It's also a valid pointer.  You still haven't shown any examples
>> of valid objects references based on an _invalid_ pointer.
>
>  My point is it is technically possible and I feel uneasy of a compiler
> creating code that's good or bad, depending on some numerical values of
> data items.

Not wanting to get too pantomime here, but my point is that it _isn't_
technically possible (or at least, that I'm not convinced that it is).
To recap, I'm asking if it's possible to:

  (1) have a symbolic address in which the final low-part daddiu could be
      affected by this bug; and

  (2) create a valid object reference using such an address.

where "valid object reference" means that code conforms to the language
standard.  Given that (1) is only satisfied for unmapped addresses,
I don't believe it is.

Basically, what I was looking for was an example of why each specific
workaround was needed.  Either there should be a testcase that would
fail without the workaround, or there should be a convincing argument
that such a testcase exists.  An example of such an argument might be:
"this testcase doesn't fail as-is, but it could if gcc did this
instead of that, and there's nothing to stop gcc from doing so".

I would be perfectly happy to accept extra workarounds if you could
justify them in this way.  (Of course, you may feel you already have. ;)

>  Please accept my apologies for taking your precious time -- I hope you 
> don't consider it completely wasted.

Now now ;)

Anyway, the patch looks OK to me, except:

> 2004-06-03  Richard Sandiford  <rsandifo@redhat.com>
> 	    Maciej W. Rozycki  <macro@ds2.pg.gda.pl>

Your name should go first.  In fact, I'm not sure mine should
be there at all.

> +(define_insn "*adddi3_internal_r4000"
> +  [(set (match_operand:DI 0 "register_operand" "=d,d")
> +	(plus:DI (match_operand:DI 1 "reg_or_0_operand" "dJ,dJ")
> +		 (match_operand:DI 2 "arith_operand" "d,Q")))]
> +  "TARGET_64BIT && !TARGET_MIPS16 && (TARGET_FIX_R4000 || TARGET_FIX_R4400)"
> +{
> +  if (which_alternative == 0)
> +    return "daddu\t%0,%z1,%2";
> +  else if ((GET_CODE (operands[1]) == REG
> +	    && REGNO (operands[1]) == STACK_POINTER_REGNUM)
> +	   || (GET_CODE (operands[1]) == CONST_INT
> +	       && (INTVAL (operands[1]) == 0)))

Use operands[1] == stack_pointer_rtx || operands[1] == const0_rtx.
Both rtxes are guaranteed to be unique.

> @@ -8409,6 +8409,9 @@ while an integer multiplication is in pr
>  @item
>  An integer division may give an incorrect result if started in a delay slot
>  of a taken branch or a jump.
> +@item
> +The @code{daddiu} instruction can produce an incorrect result (this is only
> +dealt with for a common subset of failure conditions).
>  @end itemize

Suggest:

The @code{daddiu} instruction can produce an incorrect result if the
addition overflows.

I don't think the "common subset" bit is needed since the intention is
still to prevent the use of daddiu in situations where the bug could
make a difference.

Richard