This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH][ARM] Fix broken shift patterns


> I believe this patch to be nothing but an improvement over the current
> state, and that a fix to the constraint problem should be a separate patch.
> 
> In that basis, am I OK to commit?

One minor nit:

> (define_special_predicate "shift_operator"
>...
>+	(ior (match_test "GET_CODE (XEXP (op, 1)) == CONST_INT
>+			&& ((unsigned HOST_WIDE_INT) INTVAL (XEXP (op, 1))) < 32")
>+		(match_test "REG_P (XEXP (op, 1))"))))
 
We're already enforcing the REG_P elsewhere, and it's only valid in some 
contexts, so I'd change this to:
    (match_test "GET_CODE (XEXP (op, 1)) != CONST_INT
	|| ((unsigned HOST_WIDE_INT) INTVAL (XEXP (op, 1))) < 32")


> Now, let me explain the other problem:
> 
> As it stands, all shift-related patterns, for ARM or Thumb2 mode, permit
> a shift to be expressed as either a shift type and amount (register or
> constant), or as a multiply and power-of-two constant.

Added complication is that only ARM mode accepts a register.
 
> Problem scenario 1:
> 
>    Consider pattern (plus (mult r1 r2) r3).
> 
>    It so happens that reload knows that r2 contains a constant, say 20,
>    so reload checks to see if that could be converted to an immediate.
>    Now, 20 is not a power of two, so recog would reject it, but it is in
>    the range 0..31 so it does match the 'M' constraint. Oops!

Though as you mention below we the predicate don't allow the second operand to 
be a register, so this can never happen.  Reload may do unexpected things, but 
if it starts randomly changing valid const_int values then we have much bigger 
problems.
 
> Problem scenario 2:
> 
>    Consider pattern (ashiftrt r1 r2).
> 
>    Again, it so happens that reload knows that r2 contains a constant, in
>    this case let's say 64, so again reload checks to see if that could
>    be converted to an immediate. This time, 64 is not in the range
>    0..31, so recog would reject it, but it is a power of two, so it does
>    match the 'M' constraint. Again, oops!
> 
> I see two ways to fix this properly:
> 
>   1. Duplicate all the patterns in the machine description, once for the
>      mult case, and once for the other cases. This could probably be
>      done with a code iterator, if preferred.
> 
>   2. Redefine the canonical form of (plus (mult .. 2^n) ..) such that it
>      always uses the (presumably cheaper) shift-and-add option. However,
>      this would require all other targets where madd really is the best
>      option to fix it up. (I'd imagine that two instructions for shift
>      and add would be cheaper speed wise, if properly scheduled, on most
>      targets? That doesn't help the size optimization though.)

3. Consistently accept both power-of-two and 0..31 for shifts.  Large shift 
counts give undefined results[1], so replace them with an arbitrary value
(e.g. 0) during assembly output.  Argualy not an entirely "proper" fix, but I 
think it'll keep everything happy.
 
> However, it's not obvious to me that this needs fixing:
>   * The failure mode would be an ICE, and we've not seen any.

Then again noone noticed the negative-shift ICE until recently :-/

>   * There's a comment in arm.c:shift_op that suggests that this can't
>     happen, somehow, at least in the mult case.
>     - I'm not sure exactly how reload works, but it seems reasonable
>       that it will never try to convert a register to an immediate
>       because the pattern does not allow registers in the first place.
>     - This logic doesn't hold in the opposite case though.
> Have I explained all that clearly?

I think you've convered most of it.

For bonus points we should probably disallow MULT in the arm_shiftsi3 pattern, 
stop it interacting with the regulat mulsi3 pattern in undesirable ways.

Paul

[1] Or at least not any result gcc will be expecting.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]