Re: [RS6000] PR88614, output_operand: invalid %z value

[Segher Boessenkool <segher@kernel.crashing.org>]

Hi Alan,

On Mon, Jan 07, 2019 at 09:29:18AM +1030, Alan Modra wrote:
> The direct cause of this PR is the fact that tls_gdld_nomark didn't
> handle indirect calls.  Adding the missing support revealed that most
> indirect calls were being optimised back to direct calls anyway, due
> to tls_gdld_nomark not checking any of the parallel elements except
> the first (plus the extra element that distinguishes this call from
> normal calls).  Just checking the number of elements is enough to
> separate the indirect calls from direct for ABI_ELFv2 and ABI_AIX,
> while checking for the LONG_CALL bit in the cookie works for ABI_V4.
> Direct calls being substituted for indirect calls is not the only
> unwanted substitution.  See the tls_nomark_call comment.  I also saw a
> _GLOBAL_OFFSET_TABLE_ symbol_ref being substituted for the GOT reg,
> hence the unspec_tls change.

> Bootstrap and regression testing on powerpc64le-linux and
> powerpc64-linux in progress.  Note that the patch requires
> https://gcc.gnu.org/ml/gcc-patches/2019-01/msg00252.html or the
> earlier version for the attribute support.

(Did you commit that yet?)

> +;; Verify that elements of the tls_gdld_nomark call insn parallel past the
> +;; second element (added to distinguish this call from normal calls) match
> +;; the normal contours of a call insn.  This is necessary to prevent
> +;; substitutions we don't want, for example, an indirect call being
> +;; optimised to a direct call, or (set (reg:r2) (unspec [] UNSPEC_TOCSLOT))
> +;; being cleverly optimised to (set (reg:r2) (reg:r2)) because gcc
> +;; "knows" that r2 hasn't changed from a previous call.
> +(define_predicate "tls_nomark_call"
> +  (match_code "parallel")
> +{
> +  int n = XVECLEN (op, 0);
> +  rtvec v = XVEC (op, 0);
> +  rtx set = RTVEC_ELT (v, 0);
> +  if (GET_CODE (set) != SET)
> +    return 0;
> +  rtx call = XEXP (set, 1);
> +  if (GET_CODE (call) != CALL)
> +    return 0;
> +  rtx mem = XEXP (call, 0);
> +  if (GET_CODE (mem) != MEM)
> +    return 0;
> +  rtx addr = XEXP (mem, 0);
> +  if (GET_CODE (addr) == SYMBOL_REF)
> +    {
> +      if (DEFAULT_ABI == ABI_ELFv2 || DEFAULT_ABI == ABI_AIX)
> +	return (n == 3 && GET_CODE (RTVEC_ELT (v, 2)) == CLOBBER
> +		&& REG_P (XEXP (RTVEC_ELT (v, 2), 0))
> +		&& REGNO (XEXP (RTVEC_ELT (v, 2), 0)) == LR_REGNO);
> +      else if (DEFAULT_ABI == ABI_V4)
> +	return (n >= 4 && n <= 5 && GET_CODE (RTVEC_ELT (v, 2)) == USE
> +		&& CONST_INT_P (XEXP (RTVEC_ELT (v, 2), 0))
> +		&& (INTVAL (XEXP (RTVEC_ELT (v, 2), 0)) & CALL_LONG) == 0
> +		&& (n == 4
> +		    || (GET_CODE (RTVEC_ELT (v, 3)) == USE
> +			&& REG_P (XEXP (RTVEC_ELT (v, 3), 0))))
> +		&& GET_CODE (RTVEC_ELT (v, n - 1)) == CLOBBER
> +		&& REG_P (XEXP (RTVEC_ELT (v, n - 1), 0))
> +		&& REGNO (XEXP (RTVEC_ELT (v, n - 1), 0)) == LR_REGNO);
> +      else
> +	gcc_unreachable ();
> +    }
> +  else if (indirect_call_operand (addr, mode))
> +    {
> +      if (DEFAULT_ABI == ABI_ELFv2)
> +	return (n == 4 && GET_CODE (RTVEC_ELT (v, 2)) == SET
> +		&& REG_P (XEXP (RTVEC_ELT (v, 2), 0))
> +		&& REGNO (XEXP (RTVEC_ELT (v, 2), 0)) == TOC_REGNUM
> +		&& GET_CODE (XEXP (RTVEC_ELT (v, 2), 1)) == UNSPEC
> +		&& XINT (XEXP (RTVEC_ELT (v, 2), 1), 1) == UNSPEC_TOCSLOT
> +		&& XVECLEN (XEXP (RTVEC_ELT (v, 2), 1), 0) == 1
> +		&& CONST_INT_P (XVECEXP (XEXP (RTVEC_ELT (v, 2), 1), 0, 0))
> +		&& GET_CODE (RTVEC_ELT (v, 3)) == CLOBBER
> +		&& REG_P (XEXP (RTVEC_ELT (v, 3), 0))
> +		&& REGNO (XEXP (RTVEC_ELT (v, 3), 0)) == LR_REGNO);
> +      else if (DEFAULT_ABI == ABI_AIX)
> +	return (n == 5 && GET_CODE (RTVEC_ELT (v, 2)) == USE
> +		&& GET_CODE (XEXP (RTVEC_ELT (v, 2), 0)) == MEM
> +		&& GET_CODE (RTVEC_ELT (v, 3)) == SET
> +		&& REG_P (XEXP (RTVEC_ELT (v, 3), 0))
> +		&& REGNO (XEXP (RTVEC_ELT (v, 3), 0)) == TOC_REGNUM
> +		&& GET_CODE (XEXP (RTVEC_ELT (v, 3), 1)) == UNSPEC
> +		&& XINT (XEXP (RTVEC_ELT (v, 3), 1), 1) == UNSPEC_TOCSLOT
> +		&& XVECLEN (XEXP (RTVEC_ELT (v, 3), 1), 0) == 1
> +		&& CONST_INT_P (XVECEXP (XEXP (RTVEC_ELT (v, 3), 1), 0, 0))
> +		&& GET_CODE (RTVEC_ELT (v, 4)) == CLOBBER
> +		&& REG_P (XEXP (RTVEC_ELT (v, 4), 0))
> +		&& REGNO (XEXP (RTVEC_ELT (v, 4), 0)) == LR_REGNO);
> +      else if (DEFAULT_ABI == ABI_V4)
> +	return (n == 4 && GET_CODE (RTVEC_ELT (v, 2)) == USE
> +		&& CONST_INT_P (XEXP (RTVEC_ELT (v, 2), 0))
> +		&& GET_CODE (RTVEC_ELT (v, 3)) == CLOBBER
> +		&& REG_P (XEXP (RTVEC_ELT (v, 3), 0))
> +		&& REGNO (XEXP (RTVEC_ELT (v, 3), 0)) == LR_REGNO);
> +      else
> +	gcc_unreachable ();
> +    }
> +  else
> +    return 0;
> +})

I find things like this almost impossible to read (and to verify for
correctness).  Maybe you can factor it a bit more?  Does it need to be a
predicate at all, or can you handle it by having various patterns?

> +     (plus (if_then_else (match_test "TARGET_CMODEL != CMODEL_SMALL")
> +			 (const_int 8)
> +			 (const_int 4))
> +	   (plus (if_then_else (match_test "GET_CODE (operands[1]) != SYMBOL_REF")
> +			       (plus (if_then_else (match_test "!rs6000_speculate_indirect_jumps")
> +						   (const_int 4)
> +						   (const_int 0))
> +				     (if_then_else (match_test "DEFAULT_ABI == ABI_AIX")
> +						   (const_int 4)
> +						   (const_int 0)))
> +			       (const_int 0))
> +		 (if_then_else (match_test "DEFAULT_ABI != ABI_V4")
> +			       (const_int 8)
> +			       (const_int 4)))))])

This part is way too wide.  Maybe it is would be readable if not :-(


Segher