[ARC] Fix PR89838

Hi Andrew,

This is a proposed fix for bugzilla PR89838 issue. It also needs to be backported to gcc9 and, eventually, gcc8 branches.

Ok to apply?
Claudiu

gcc/
xxxx-xx-xx  Claudiu Zissulescu  <claziss@synopsys.com>

	* config/arc/arc.c (arc_symbol_binds_local_p): New function.
	(arc_legitimize_pic_address): Simplify and cleanup the function.
	(SYMBOLIC_CONST): Remove.
	(prepare_pic_move): Likewise.
	(prepare_move_operands): Handle complex mov cases here.
	(arc_legitimize_address_0): Remove call to
	arc_legitimize_pic_address.
	(arc_legitimize_address): Remove call to
	arc_legitimize_tls_address.
	* config/arc/arc.md (movqi_insn): Allow Cm3 match.
	(movhi_insn): Likewise.

/gcc/testsuite
xxxx-xx-xx  Claudiu Zissulescu  <claziss@synopsys.com>

	* gcc.target/arc/pr89838.c: New file.
---
 gcc/config/arc/arc.c                   | 215 +++++++------------------
 gcc/config/arc/arc.md                  |   8 +-
 gcc/testsuite/gcc.target/arc/pr89838.c |  16 ++
 3 files changed, 77 insertions(+), 162 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/arc/pr89838.c

On 6/6/19 1:32 AM, Claudiu Zissulescu wrote:
> Hi Andrew,
> 
> This is a proposed fix for bugzilla PR89838 issue. It also needs to be backported to gcc9 and, eventually, gcc8 branches.
> 
> Ok to apply?
> Claudiu
> 
> gcc/
> xxxx-xx-xx  Claudiu Zissulescu  <claziss@synopsys.com>
> 
> 	* config/arc/arc.c (arc_symbol_binds_local_p): New function.
> 	(arc_legitimize_pic_address): Simplify and cleanup the function.
> 	(SYMBOLIC_CONST): Remove.
> 	(prepare_pic_move): Likewise.
> 	(prepare_move_operands): Handle complex mov cases here.
> 	(arc_legitimize_address_0): Remove call to
> 	arc_legitimize_pic_address.
> 	(arc_legitimize_address): Remove call to
> 	arc_legitimize_tls_address.
> 	* config/arc/arc.md (movqi_insn): Allow Cm3 match.
> 	(movhi_insn): Likewise.
> 
> /gcc/testsuite
> xxxx-xx-xx  Claudiu Zissulescu  <claziss@synopsys.com>
> 
> 	* gcc.target/arc/pr89838.c: New file.
OK.

THe BZ mentions that this was found building a glibc test for ARC.  Is
there a glibc port for the ARC?  I don't see one in the glibc git repo.
 Are you aware of any plans to produce an official glibc port.

I believe building glibc is a hell of a better sniff test than building
newlib.  So if it's in the plan, I'd love to re-wire my tester to test
with glibc rather than newlib on the ARC port.

jeff

Hi Jeff,

> OK.
> 
 Thank you for your review.

> THe BZ mentions that this was found building a glibc test for ARC.  Is
> there a glibc port for the ARC?  I don't see one in the glibc git repo.
>  Are you aware of any plans to produce an official glibc port.

Indeed, we are in the process of upstreaming the glibc port for arc. However,  we hit a number of issues, one being this BZ, and other is the need to provide some ARC processor internals documentation. Once those items cleaned up, the arc glibc port will be public available upstream.

> 
> I believe building glibc is a hell of a better sniff test than building
> newlib.  So if it's in the plan, I'd love to re-wire my tester to test
> with glibc rather than newlib on the ARC port.
> 

I totally agree, we also have an uClibc-ng port which, as far as I know, is fully upstream. If I may ask, how do you test the ARC toolchain, do you use Synopsys free nSIM simulator?

Thank you,
Claudiu

On 6/19/19 3:29 AM, Claudiu Zissulescu wrote:
> Hi Jeff,
> 
>> OK.
>> 
> Thank you for your review.
> 
>> THe BZ mentions that this was found building a glibc test for ARC.
>> Is there a glibc port for the ARC?  I don't see one in the glibc
>> git repo. Are you aware of any plans to produce an official glibc
>> port.
> 
> Indeed, we are in the process of upstreaming the glibc port for arc.
> However,  we hit a number of issues, one being this BZ, and other is
> the need to provide some ARC processor internals documentation. Once
> those items cleaned up, the arc glibc port will be public available
> upstream.
That'll be great.  It looks like there's an ARC kernel port (needed for
the headers and kernel build step).  So it should look like a standard
cross glibc/kernel as far as my tester is concerned.


> 
>> 
>> I believe building glibc is a hell of a better sniff test than
>> building newlib.  So if it's in the plan, I'd love to re-wire my
>> tester to test with glibc rather than newlib on the ARC port.
>> 
> 
> I totally agree, we also have an uClibc-ng port which, as far as I
> know, is fully upstream. If I may ask, how do you test the ARC
> toolchain, do you use Synopsys free nSIM simulator?
I've pondered wiring in support for uClibc, but haven't had the time.
Right now for ARC we build libgcc, and newlib/libgloss, then run the gcc
testsuite with a dummy simulator.

Essentially this verifies the compiler generates code that can be
assembled and linked and allows us to also use the tests which scan dumps.

The dummy simulator always returns success to avoid useless noise.

The state of simulated targets using newlib/libgloss is generally poor.
 A large part of the problem is the fixed, limited, memory size that's
baked into the libgloss linker scripts.  This results in numerous
clashes of the heap & stack and whether or not those cause an execution
failure is horribly unpredictable, so except for a couple ports I
audited, I'm not doing simulator testing for the newlib/libgloss targets.

jeff

* Claudiu Zissulescu <claziss@gmail.com> [2019-06-06 10:32:11 +0300]:

> Hi Andrew,
> 
> This is a proposed fix for bugzilla PR89838 issue. It also needs to be backported to gcc9 and, eventually, gcc8 branches.
> 
> Ok to apply?
> Claudiu

I had a look through the patch and found just one small nit detailed
below.  Otherwise this looks fine.

Thanks,
Andrew

> 
> gcc/
> xxxx-xx-xx  Claudiu Zissulescu  <claziss@synopsys.com>
> 
> 	* config/arc/arc.c (arc_symbol_binds_local_p): New function.
> 	(arc_legitimize_pic_address): Simplify and cleanup the function.
> 	(SYMBOLIC_CONST): Remove.
> 	(prepare_pic_move): Likewise.
> 	(prepare_move_operands): Handle complex mov cases here.
> 	(arc_legitimize_address_0): Remove call to
> 	arc_legitimize_pic_address.
> 	(arc_legitimize_address): Remove call to
> 	arc_legitimize_tls_address.
> 	* config/arc/arc.md (movqi_insn): Allow Cm3 match.
> 	(movhi_insn): Likewise.
> 
> /gcc/testsuite
> xxxx-xx-xx  Claudiu Zissulescu  <claziss@synopsys.com>
> 
> 	* gcc.target/arc/pr89838.c: New file.
> ---
>  gcc/config/arc/arc.c                   | 215 +++++++------------------
>  gcc/config/arc/arc.md                  |   8 +-
>  gcc/testsuite/gcc.target/arc/pr89838.c |  16 ++
>  3 files changed, 77 insertions(+), 162 deletions(-)
>  create mode 100644 gcc/testsuite/gcc.target/arc/pr89838.c
> 
> diff --git a/gcc/config/arc/arc.c b/gcc/config/arc/arc.c
> index 20dfae66370..a5ee5c49a8f 100644
> --- a/gcc/config/arc/arc.c
> +++ b/gcc/config/arc/arc.c
> @@ -5986,130 +5986,47 @@ arc_legitimize_tls_address (rtx addr, enum tls_model model)
>      }
>  }
>  
> -/* Legitimize a pic address reference in ORIG.
> -   The return value is the legitimated address.
> -   If OLDX is non-zero, it is the target to assign the address to first.  */
> +/* Return true if SYMBOL_REF X binds locally.  */
>  
> -static rtx
> -arc_legitimize_pic_address (rtx orig, rtx oldx)
> +static bool
> +arc_symbol_binds_local_p (const_rtx x)
>  {
> -  rtx addr = orig;
> -  rtx pat = orig;
> -  rtx base;
> +  return (SYMBOL_REF_DECL (x)
> +	  ? targetm.binds_local_p (SYMBOL_REF_DECL (x))
> +	  : SYMBOL_REF_LOCAL_P (x));
> +}
> +
> +/* Legitimize a pic address reference in ORIG.  The return value is
> +   the legitimated address.  */

This comment needs updating I think, it references ORIG, but there is
no ORIG in arc_legitimize_pic.

>  
> -  if (oldx == orig)
> -    oldx = NULL;
> +static rtx
> +arc_legitimize_pic_address (rtx addr)
> +{
> +  if (!flag_pic)
> +    return addr;
>  
> -  if (GET_CODE (addr) == LABEL_REF)
> -    ; /* Do nothing.  */
> -  else if (GET_CODE (addr) == SYMBOL_REF)
> +  switch (GET_CODE (addr))
>      {
> -      enum tls_model model = SYMBOL_REF_TLS_MODEL (addr);
> -      if (model != 0)
> -	return arc_legitimize_tls_address (addr, model);
> -      else if (!flag_pic)
> -	return orig;
> -      else if (CONSTANT_POOL_ADDRESS_P (addr) || SYMBOL_REF_LOCAL_P (addr))
> -	return arc_unspec_offset (addr, ARC_UNSPEC_GOTOFFPC);
> +    case SYMBOL_REF:
> +      /* TLS symbols are handled in different place.  */
> +      if (SYMBOL_REF_TLS_MODEL (addr))
> +	return addr;
>  
>        /* This symbol must be referenced via a load from the Global
>  	 Offset Table (@GOTPC).  */
> -      pat = arc_unspec_offset (addr, ARC_UNSPEC_GOT);
> -      pat = gen_const_mem (Pmode, pat);
> -
> -      if (oldx == NULL)
> -	oldx = gen_reg_rtx (Pmode);
> -
> -      emit_move_insn (oldx, pat);
> -      pat = oldx;
> -    }
> -  else
> -    {
> -      if (GET_CODE (addr) == CONST)
> -	{
> -	  addr = XEXP (addr, 0);
> -	  if (GET_CODE (addr) == UNSPEC)
> -	    {
> -	      /* Check that the unspec is one of the ones we generate?  */
> -	      return orig;
> -	    }
> -	  /* fwprop is placing in the REG_EQUIV notes constant pic
> -	     unspecs expressions.  Then, loop may use these notes for
> -	     optimizations resulting in complex patterns that are not
> -	     supported by the current implementation. The following
> -	     two if-cases are simplifying the complex patters to
> -	     simpler ones.  */
> -	  else if (GET_CODE (addr) == MINUS)
> -	    {
> -	      rtx op0 = XEXP (addr, 0);
> -	      rtx op1 = XEXP (addr, 1);
> -	      gcc_assert (oldx);
> -	      gcc_assert (GET_CODE (op1) == UNSPEC);
> -
> -	      emit_move_insn (oldx,
> -			      gen_rtx_CONST (SImode,
> -					     arc_legitimize_pic_address (op1,
> -									 NULL_RTX)));
> -	      emit_insn (gen_rtx_SET (oldx, gen_rtx_MINUS (SImode, op0, oldx)));
> -	      return oldx;
> -
> -	    }
> -	  else if (GET_CODE (addr) != PLUS)
> -	    {
> -	      rtx tmp = XEXP (addr, 0);
> -	      enum rtx_code code = GET_CODE (addr);
> -
> -	      /* It only works for UNARY operations.  */
> -	      gcc_assert (UNARY_P (addr));
> -	      gcc_assert (GET_CODE (tmp) == UNSPEC);
> -	      gcc_assert (oldx);
> -
> -	      emit_move_insn
> -		(oldx,
> -		 gen_rtx_CONST (SImode,
> -				arc_legitimize_pic_address (tmp,
> -							    NULL_RTX)));
> -
> -	      emit_insn (gen_rtx_SET (oldx,
> -				      gen_rtx_fmt_ee (code, SImode,
> -						      oldx, const0_rtx)));
> -
> -	      return oldx;
> -	    }
> -	  else
> -	    {
> -	      gcc_assert (GET_CODE (addr) == PLUS);
> -	      if (GET_CODE (XEXP (addr, 0)) == UNSPEC)
> -		return orig;
> -	    }
> -	}
> -
> -      if (GET_CODE (addr) == PLUS)
> -	{
> -	  rtx op0 = XEXP (addr, 0), op1 = XEXP (addr, 1);
> -
> -	  base = arc_legitimize_pic_address (op0, oldx);
> -	  pat  = arc_legitimize_pic_address (op1,
> -					     base == oldx ? NULL_RTX : oldx);
> +      if (!arc_symbol_binds_local_p (addr))
> +	return gen_const_mem (Pmode, arc_unspec_offset (addr, ARC_UNSPEC_GOT));
>  
> -	  if (base == op0 && pat == op1)
> -	    return orig;
> +      /* Local symb: use @pcl to access it.  */
> +      /* Fall through.  */
> +    case LABEL_REF:
> +      return arc_unspec_offset (addr, ARC_UNSPEC_GOTOFFPC);
>  
> -	  if (GET_CODE (pat) == CONST_INT)
> -	    pat = plus_constant (Pmode, base, INTVAL (pat));
> -	  else
> -	    {
> -	      if (GET_CODE (pat) == PLUS && CONSTANT_P (XEXP (pat, 1)))
> -		{
> -		  base = gen_rtx_PLUS (Pmode, base, XEXP (pat, 0));
> -		  pat = XEXP (pat, 1);
> -		}
> -	      pat = gen_rtx_PLUS (Pmode, base, pat);
> -	    }
> -	}
> +    default:
> +      break;
>      }
>  
> - return pat;
> + return addr;
>  }
>  
>  /* Output address constant X to FILE, taking PIC into account.  */
> @@ -6271,28 +6188,6 @@ arc_output_pic_addr_const (FILE * file, rtx x, int code)
>      }
>  }
>  
> -#define SYMBOLIC_CONST(X)	\
> -(GET_CODE (X) == SYMBOL_REF						\
> - || GET_CODE (X) == LABEL_REF						\
> - || (GET_CODE (X) == CONST && symbolic_reference_mentioned_p (X)))
> -
> -/* Emit insns to move operands[1] into operands[0].  */
> -
> -static void
> -prepare_pic_move (rtx *operands, machine_mode)
> -{
> -  if (GET_CODE (operands[0]) == MEM && SYMBOLIC_CONST (operands[1])
> -      && flag_pic)
> -    operands[1] = force_reg (Pmode, operands[1]);
> -  else
> -    {
> -      rtx temp = (reload_in_progress ? operands[0]
> -		  : flag_pic? gen_reg_rtx (Pmode) : NULL_RTX);
> -      operands[1] = arc_legitimize_pic_address (operands[1], temp);
> -    }
> -}
> -
> -
>  /* The function returning the number of words, at the beginning of an
>     argument, must be put in registers.  The returned value must be
>     zero for arguments that are passed entirely in registers or that
> @@ -9072,30 +8967,38 @@ prepare_move_operands (rtx *operands, machine_mode mode)
>  	}
>      }
>  
> -  if (mode == SImode && SYMBOLIC_CONST (operands[1]))
> +  if (GET_CODE (operands[1]) == SYMBOL_REF)
>      {
> -      prepare_pic_move (operands, SImode);
> -
> -      /* Disable any REG_EQUALs associated with the symref
> -	 otherwise the optimization pass undoes the work done
> -	 here and references the variable directly.  */
> +      enum tls_model model = SYMBOL_REF_TLS_MODEL (operands[1]);
> +      if (MEM_P (operands[0]) && flag_pic)
> +	operands[1] = force_reg (mode, operands[1]);
> +      else if (model)
> +	operands[1] = arc_legitimize_tls_address (operands[1], model);
>      }
>  
> +  operands[1] = arc_legitimize_pic_address (operands[1]);
> +
> +  /* Store instructions are limited, they only accept as address an
> +     immediate, a register or a register plus a small immediate.  */
>    if (MEM_P (operands[0])
> -      && !(reload_in_progress || reload_completed))
> +      && !move_dest_operand (operands[0], mode))
>      {
> -      operands[1] = force_reg (mode, operands[1]);
> -      if (!move_dest_operand (operands[0], mode))
> -	{
> -	  rtx addr = copy_to_mode_reg (Pmode, XEXP (operands[0], 0));
> -	  /* This is like change_address_1 (operands[0], mode, 0, 1) ,
> -	     except that we can't use that function because it is static.  */
> -	  rtx pat = change_address (operands[0], mode, addr);
> -	  MEM_COPY_ATTRIBUTES (pat, operands[0]);
> -	  operands[0] = pat;
> -	}
> +      rtx tmp0 = copy_to_mode_reg (Pmode, XEXP (operands[0], 0));
> +      rtx tmp1 = change_address (operands[0], mode, tmp0);
> +      MEM_COPY_ATTRIBUTES (tmp1, operands[0]);
> +      operands[0] = tmp1;
>      }
>  
> +  /* Check if it is constant but it is not legitimized.  */
> +  if (CONSTANT_P (operands[1])
> +      && !arc_legitimate_constant_p (mode, operands[1]))
> +    operands[1] = force_reg (mode, XEXP (operands[1], 0));
> +  else if (MEM_P (operands[0])
> +	   && ((CONSTANT_P (operands[1])
> +		&& !satisfies_constraint_Cm3 (operands[1]))
> +	       || MEM_P (operands[1])))
> +    operands[1] = force_reg (mode, operands[1]);
> +
>    return false;
>  }
>  
> @@ -9566,11 +9469,10 @@ arc_legitimize_address_0 (rtx x, rtx oldx ATTRIBUTE_UNUSED,
>  {
>    rtx addr, inner;
>  
> -  if (flag_pic && SYMBOLIC_CONST (x))
> -     (x) =  arc_legitimize_pic_address (x, 0);
>    addr = x;
>    if (GET_CODE (addr) == CONST)
>      addr = XEXP (addr, 0);
> +
>    if (GET_CODE (addr) == PLUS
>        && CONST_INT_P (XEXP (addr, 1))
>        && ((GET_CODE (XEXP (addr, 0)) == SYMBOL_REF
> @@ -9601,13 +9503,6 @@ arc_legitimize_address_0 (rtx x, rtx oldx ATTRIBUTE_UNUSED,
>  static rtx
>  arc_legitimize_address (rtx orig_x, rtx oldx, machine_mode mode)
>  {
> -  if (GET_CODE (orig_x) == SYMBOL_REF)
> -    {
> -      enum tls_model model = SYMBOL_REF_TLS_MODEL (orig_x);
> -      if (model != 0)
> -	return arc_legitimize_tls_address (orig_x, model);
> -    }
> -
>    rtx new_x = arc_legitimize_address_0 (orig_x, oldx, mode);
>  
>    if (new_x)
> diff --git a/gcc/config/arc/arc.md b/gcc/config/arc/arc.md
> index 082b5bba6ec..ed29aa3d06e 100644
> --- a/gcc/config/arc/arc.md
> +++ b/gcc/config/arc/arc.md
> @@ -672,7 +672,9 @@ core_3, archs4x, archs4xd, archs4xd_slow"
>    [(set (match_operand:QI 0 "move_dest_operand" "=Rcq,Rcq#q,    w,Rcq#q,   h, w, w,???w,h, w,Rcq,  S,!*x,  r,r, Ucm,m,???m,  m,Usc")
>  	(match_operand:QI 1 "move_src_operand"  "  cL,   cP,Rcq#q,    P,hCm1,cL, I,?Rac,i,?i,  T,Rcq,Usd,Ucm,m,?Rac,c,?Rac,Cm3,i"))]
>    "register_operand (operands[0], QImode)
> -   || register_operand (operands[1], QImode)"
> +   || register_operand (operands[1], QImode)
> +   || (satisfies_constraint_Cm3 (operands[1])
> +       && memory_operand (operands[0], QImode))"
>    "@
>     mov%? %0,%1%&
>     mov%? %0,%1%&
> @@ -714,7 +716,9 @@ core_3, archs4x, archs4xd, archs4xd_slow"
>         /* Don't use a LIMM that we could load with a single insn - we loose
>  	  delay-slot filling opportunities.  */
>         && !satisfies_constraint_I (operands[1])
> -       && satisfies_constraint_Usc (operands[0]))"
> +       && satisfies_constraint_Usc (operands[0]))
> +   || (satisfies_constraint_Cm3 (operands[1])
> +       && memory_operand (operands[0], HImode))"
>    "@
>     mov%? %0,%1%&
>     mov%? %0,%1%&
> diff --git a/gcc/testsuite/gcc.target/arc/pr89838.c b/gcc/testsuite/gcc.target/arc/pr89838.c
> new file mode 100644
> index 00000000000..559434ac87e
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/arc/pr89838.c
> @@ -0,0 +1,16 @@
> +/* { dg-do compile } */
> +/* { dg-require-effective-target tls } */
> +/* { dg-options "-O2" } */
> +
> +extern void foo (void);
> +extern void bar (void *);
> +
> +struct {
> +  int __attribute__(()) a;
> +  int __attribute__(()) b;
> +} __thread c __attribute__((tls_model("initial-exec")));
> +
> +void foo (void)
> +{
> +  bar (&c.b);
> +}
> -- 
> 2.21.0
>

Thank you guys for your review. Patch pushed to master.

Claudiu