RE: [PATCH][ARM]Tighten the conditions for arm_movw, arm_movt

["Kyrill Tkachov" <kyrylo dot tkachov at arm dot com>]

Hi Renlin,

Please send patches to gcc-patches for review.
Redirecting there now...


On 19/08/15 12:49, Renlin Li wrote:
> Hi all,
>
> This simple patch will tighten the conditions when matching movw and
> arm_movt rtx pattern.
> Those two patterns will generate the following assembly:
>
> movw w1, #:lower16: dummy + addend
> movt w1, #:upper16: dummy + addend
>
> The addend here is optional. However, it should be an 16-bit signed
> value with in the range -32768 <= A <= 32768.
>
> By impose this restriction explicitly, it will prevent LRA/reload code
> from generation invalid high/lo_sum code for arm target.
> In process_address_1(), if the address is not legitimate, it will try to
> generate high/lo_sum pair to put the address into register. It will
> check if the target support those newly generated reload instructions.
> By define those two patterns, arm will reject them if conditions is not
> meet.
>
> Otherwise, it might generate movw/movt instructions with addend larger
> than 32768, this will cause a GAS error. GAS will produce '''offset out
> of range'' error message when the addend for MOVW/MOVT REL relocation is
> too large.
>
>
> arm-none-eabi regression tests Okay, Okay to commit to the trunk and
> backport to 5.0?
>
> Regards,
> Renlin
>
> gcc/ChangeLog:
>
> 2015-08-19  Renlin Li  <renlin.li@arm.com>
>
>       * config/arm/arm-protos.h (arm_valid_symbolic_address_p): Declare.
>       * config/arm/arm.c (arm_valid_symbolic_address_p): Define.
>       * config/arm/arm.md (arm_movt): Use arm_valid_symbolic_address_p.
>       * config/arm/constraints.md ("j"): Add check for high code.

+/* Returns true if the pattern is a valid symbolic address, which is either a
+   symbol_ref or a symbol_ref + offset.  */
+bool
+arm_valid_symbolic_address_p (rtx addr)

New line between comment and function.

+{
+  rtx xop0, xop1 = NULL_RTX;
+  rtx tmp = addr;
+
+  if (GET_CODE (tmp) == SYMBOL_REF || GET_CODE (tmp) == LABEL_REF)
+    return true;
+
+  /* (const (plus: symbol_ref const_int))  */
+  if (GET_CODE (addr) == CONST)
+    tmp = XEXP (addr, 0);
+
+  xop0 = XEXP (tmp, 0);
+  xop1 = XEXP (tmp, 1);


Is it guaranteed that at this point XEXP (tmp, 0) and XEXP (tmp, 1) are valid?
I think before you extract xop0 and xop1 you want to check that tmp is indeed a PLUS
and return false if it's not. Only then you should extract XEXP (tmp, 0) and XEXP (tmp, 1).

 +  if (GET_CODE (tmp) == PLUS && GET_CODE (xop0) == SYMBOL_REF
+      && CONST_INT_P (xop1))
+    {
+      HOST_WIDE_INT offset = INTVAL (xop1);
+      if (offset < -0x8000 || offset > 0x7fff)
+	return false;
+      else
+	return true;

I think you can just do "return IN_RANGE (offset, -0x8000, 0x7ffff);"

Thanks,
Kyrill