Patchwork Don't return original value from gen_lowpart_no_emit rtl hook (PR debug/55717)

Hi!

If gen_lowpart_if_possible returns NULL, the default
rtl_hooks.gen_lowpart_no_emit hook returns the original value, which is not
of the desired mode.  I bet in most passes for real insns such rtx is then
meant to fail recog and thrown away, but for DEBUG_INSN modification that
doesn't work, since there is no verification (but also e.g. any kind of
SUBREG is fine).  So we can e.g. end up with a (plus:SI (mem:DI ...) (mem:SI ...))
or similar and then various passes (in this testcase on s390x reload) can be
very upset about that.

Fixed by making the hook instead return NULL_RTX if it wasn't possible to
generate the lowpart, and adjusting all the hook uses to cope with that.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2012-12-17  Jakub Jelinek  <jakub@redhat.com>

	PR debug/55717
	* rtlhooks-def.h (RTL_HOOKS_GEN_LOWPART_NO_EMIT): Define to
	gen_lowpart_if_possible.
	(gen_lowpart_no_emit_general): Remove prototype.
	* rtlhooks.c (gen_lowpart_no_emit_general): Removed.
	* simplify-rtx.c (simplify_unary_operation_1, 
	simplify_binary_operation_1): Continue simplifying if
	rtl_hooks.gen_lowpart_no_emit returns NULL_RTX.
	* dwarf2out.c (mem_loc_descriptor) <case TRUNCATE>: Handle
	truncation like lowpart SUBREG.

	* testsuite/g++.dg/opt/pr55717.C: New test.


	Jakub

Il 17/12/2012 22:33, Jakub Jelinek ha scritto:
> Hi!
> 
> If gen_lowpart_if_possible returns NULL, the default
> rtl_hooks.gen_lowpart_no_emit hook returns the original value, which is not
> of the desired mode.  I bet in most passes for real insns such rtx is then
> meant to fail recog and thrown away, but for DEBUG_INSN modification that
> doesn't work, since there is no verification (but also e.g. any kind of
> SUBREG is fine).  So we can e.g. end up with a (plus:SI (mem:DI ...) (mem:SI ...))
> or similar and then various passes (in this testcase on s390x reload) can be
> very upset about that.

Makes sense, and it could even be a wrong-code bug for this simplification:

      /* (sign_extend:M (ashiftrt:N (ashift <X> (const_int I)) (const_int I)))
         is (sign_extend:M (subreg:O <X>)) if there is mode with
         GET_MODE_BITSIZE (N) - I bits.
         (sign_extend:M (lshiftrt:N (ashift <X> (const_int I)) (const_int I)))
         is similarly (zero_extend:M (subreg:O <X>)).  */
      if ((GET_CODE (op) == ASHIFTRT || GET_CODE (op) == LSHIFTRT)
          && GET_CODE (XEXP (op, 0)) == ASHIFT
          && CONST_INT_P (XEXP (op, 1))
          && XEXP (XEXP (op, 0), 1) == XEXP (op, 1)
          && GET_MODE_BITSIZE (GET_MODE (op)) > INTVAL (XEXP (op, 1)))
        {
          enum machine_mode tmode
            = mode_for_size (GET_MODE_BITSIZE (GET_MODE (op))
                             - INTVAL (XEXP (op, 1)), MODE_INT, 1);
          gcc_assert (GET_MODE_BITSIZE (mode)
                      > GET_MODE_BITSIZE (GET_MODE (op)));
          if (tmode != BLKmode)
            {
              rtx inner =
                rtl_hooks.gen_lowpart_no_emit (tmode, XEXP (XEXP (op, 0), 0));
              if (!inner)
                return NULL_RTX;
              return simplify_gen_unary (GET_CODE (op) == ASHIFTRT
                                         ? SIGN_EXTEND : ZERO_EXTEND,
                                         mode, inner, tmode);
            }
        }

So perhaps we want it on stable branches too, after some time in trunk.

Paolo

> Fixed by making the hook instead return NULL_RTX if it wasn't possible to
> generate the lowpart, and adjusting all the hook uses to cope with that.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> 
> 2012-12-17  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR debug/55717
> 	* rtlhooks-def.h (RTL_HOOKS_GEN_LOWPART_NO_EMIT): Define to
> 	gen_lowpart_if_possible.
> 	(gen_lowpart_no_emit_general): Remove prototype.
> 	* rtlhooks.c (gen_lowpart_no_emit_general): Removed.
> 	* simplify-rtx.c (simplify_unary_operation_1, 
> 	simplify_binary_operation_1): Continue simplifying if
> 	rtl_hooks.gen_lowpart_no_emit returns NULL_RTX.
> 	* dwarf2out.c (mem_loc_descriptor) <case TRUNCATE>: Handle
> 	truncation like lowpart SUBREG.
> 
> 	* testsuite/g++.dg/opt/pr55717.C: New test.
> 
> --- gcc/rtlhooks-def.h.jj	2009-02-20 15:40:11.000000000 +0100
> +++ gcc/rtlhooks-def.h	2012-12-17 19:34:38.852895461 +0100
> @@ -23,7 +23,7 @@ along with GCC; see the file COPYING3.
>  #include "rtl.h"
>  
>  #define RTL_HOOKS_GEN_LOWPART gen_lowpart_general
> -#define RTL_HOOKS_GEN_LOWPART_NO_EMIT gen_lowpart_no_emit_general
> +#define RTL_HOOKS_GEN_LOWPART_NO_EMIT gen_lowpart_if_possible
>  #define RTL_HOOKS_REG_NONZERO_REG_BITS reg_nonzero_bits_general
>  #define RTL_HOOKS_REG_NUM_SIGN_BIT_COPIES reg_num_sign_bit_copies_general
>  #define RTL_HOOKS_REG_TRUNCATED_TO_MODE reg_truncated_to_mode_general
> @@ -38,7 +38,6 @@ along with GCC; see the file COPYING3.
>  }
>  
>  extern rtx gen_lowpart_general (enum machine_mode, rtx);
> -extern rtx gen_lowpart_no_emit_general (enum machine_mode, rtx);
>  extern rtx reg_nonzero_bits_general (const_rtx, enum machine_mode, const_rtx,
>  				     enum machine_mode,
>  				     unsigned HOST_WIDE_INT,
> --- gcc/rtlhooks.c.jj	2011-07-07 13:24:05.000000000 +0200
> +++ gcc/rtlhooks.c	2012-12-17 19:34:08.634071165 +0100
> @@ -79,18 +79,6 @@ gen_lowpart_general (enum machine_mode m
>      }
>  }
>  
> -/* Similar to gen_lowpart, but cannot emit any instruction via
> -   copy_to_reg or force_reg.  Mainly used in simplify-rtx.c.  */
> -rtx
> -gen_lowpart_no_emit_general (enum machine_mode mode, rtx x)
> -{
> -  rtx result = gen_lowpart_if_possible (mode, x);
> -  if (result)
> -    return result;
> -  else
> -    return x;
> -}
> -
>  rtx
>  reg_num_sign_bit_copies_general (const_rtx x ATTRIBUTE_UNUSED,
>  				 enum machine_mode mode ATTRIBUTE_UNUSED,
> --- gcc/simplify-rtx.c.jj	2012-11-28 23:57:47.000000000 +0100
> +++ gcc/simplify-rtx.c	2012-12-17 18:10:48.873398378 +0100
> @@ -873,7 +873,9 @@ simplify_unary_operation_1 (enum rtx_cod
>  			      simplify_gen_unary (NOT, inner_mode, const1_rtx,
>  						  inner_mode),
>  			      XEXP (SUBREG_REG (op), 1));
> -	  return rtl_hooks.gen_lowpart_no_emit (mode, x);
> +	  temp = rtl_hooks.gen_lowpart_no_emit (mode, x);
> +	  if (temp)
> +	    return temp;
>  	}
>  
>        /* Apply De Morgan's laws to reduce number of patterns for machines
> @@ -1029,7 +1031,11 @@ simplify_unary_operation_1 (enum rtx_cod
>        if (GET_MODE_CLASS (mode) == MODE_PARTIAL_INT)
>  	{
>  	  if (TRULY_NOOP_TRUNCATION_MODES_P (mode, GET_MODE (op)))
> -	    return rtl_hooks.gen_lowpart_no_emit (mode, op);
> +	    {
> +	      temp = rtl_hooks.gen_lowpart_no_emit (mode, op);
> +	      if (temp)
> +		return temp;
> +	    }
>  	  /* We can't handle truncation to a partial integer mode here
>  	     because we don't know the real bitsize of the partial
>  	     integer mode.  */
> @@ -1048,7 +1054,11 @@ simplify_unary_operation_1 (enum rtx_cod
>        if (GET_MODE_NUNITS (mode) == 1
>  	  && (TRULY_NOOP_TRUNCATION_MODES_P (mode, GET_MODE (op))
>  	      || truncated_to_mode (mode, op)))
> -	return rtl_hooks.gen_lowpart_no_emit (mode, op);
> +	{
> +	  temp = rtl_hooks.gen_lowpart_no_emit (mode, op);
> +	  if (temp)
> +	    return temp;
> +	}
>  
>        /* A truncate of a comparison can be replaced with a subreg if
>           STORE_FLAG_VALUE permits.  This is like the previous test,
> @@ -1057,7 +1067,11 @@ simplify_unary_operation_1 (enum rtx_cod
>        if (HWI_COMPUTABLE_MODE_P (mode)
>  	  && COMPARISON_P (op)
>  	  && (STORE_FLAG_VALUE & ~GET_MODE_MASK (mode)) == 0)
> -	return rtl_hooks.gen_lowpart_no_emit (mode, op);
> +	{
> +	  temp = rtl_hooks.gen_lowpart_no_emit (mode, op);
> +	  if (temp)
> +	    return temp;
> +	}
>  
>        /* A truncate of a memory is just loading the low part of the memory
>  	 if we are not changing the meaning of the address. */
> @@ -1065,7 +1079,11 @@ simplify_unary_operation_1 (enum rtx_cod
>  	  && !VECTOR_MODE_P (mode)
>  	  && !MEM_VOLATILE_P (op)
>  	  && !mode_dependent_address_p (XEXP (op, 0), MEM_ADDR_SPACE (op)))
> -	return rtl_hooks.gen_lowpart_no_emit (mode, op);
> +	{
> +	  temp = rtl_hooks.gen_lowpart_no_emit (mode, op);
> +	  if (temp)
> +	    return temp;
> +	}
>  
>        break;
>  
> @@ -1298,7 +1316,11 @@ simplify_unary_operation_1 (enum rtx_cod
>  	  && SUBREG_PROMOTED_VAR_P (op)
>  	  && ! SUBREG_PROMOTED_UNSIGNED_P (op)
>  	  && GET_MODE_SIZE (mode) <= GET_MODE_SIZE (GET_MODE (XEXP (op, 0))))
> -	return rtl_hooks.gen_lowpart_no_emit (mode, op);
> +	{
> +	  temp = rtl_hooks.gen_lowpart_no_emit (mode, op);
> +	  if (temp)
> +	    return temp;
> +	}
>  
>        /* (sign_extend:M (sign_extend:N <X>)) is (sign_extend:M <X>).
>  	 (sign_extend:M (zero_extend:N <X>)) is (zero_extend:M <X>).  */
> @@ -1330,9 +1352,10 @@ simplify_unary_operation_1 (enum rtx_cod
>  	    {
>  	      rtx inner =
>  		rtl_hooks.gen_lowpart_no_emit (tmode, XEXP (XEXP (op, 0), 0));
> -	      return simplify_gen_unary (GET_CODE (op) == ASHIFTRT
> -					 ? SIGN_EXTEND : ZERO_EXTEND,
> -					 mode, inner, tmode);
> +	      if (inner)
> +		return simplify_gen_unary (GET_CODE (op) == ASHIFTRT
> +					   ? SIGN_EXTEND : ZERO_EXTEND,
> +					   mode, inner, tmode);
>  	    }
>  	}
>  
> @@ -1360,7 +1383,11 @@ simplify_unary_operation_1 (enum rtx_cod
>  	  && SUBREG_PROMOTED_VAR_P (op)
>  	  && SUBREG_PROMOTED_UNSIGNED_P (op) > 0
>  	  && GET_MODE_SIZE (mode) <= GET_MODE_SIZE (GET_MODE (XEXP (op, 0))))
> -	return rtl_hooks.gen_lowpart_no_emit (mode, op);
> +	{
> +	  temp = rtl_hooks.gen_lowpart_no_emit (mode, op);
> +	  if (temp)
> +	    return temp;
> +	}
>  
>        /* Extending a widening multiplication should be canonicalized to
>  	 a wider widening multiplication.  */
> @@ -1425,7 +1452,8 @@ simplify_unary_operation_1 (enum rtx_cod
>  	    {
>  	      rtx inner =
>  		rtl_hooks.gen_lowpart_no_emit (tmode, XEXP (XEXP (op, 0), 0));
> -	      return simplify_gen_unary (ZERO_EXTEND, mode, inner, tmode);
> +	      if (inner)
> +		return simplify_gen_unary (ZERO_EXTEND, mode, inner, tmode);
>  	    }
>  	}
>  
> @@ -3095,7 +3123,11 @@ simplify_binary_operation_1 (enum rtx_co
>  	}
>        /* x/1 is x.  */
>        if (trueop1 == CONST1_RTX (mode))
> -	return rtl_hooks.gen_lowpart_no_emit (mode, op0);
> +	{
> +	  tem = rtl_hooks.gen_lowpart_no_emit (mode, op0);
> +	  if (tem)
> +	    return tem;
> +	}
>        /* Convert divide by power of two into shift.  */
>        if (CONST_INT_P (trueop1)
>  	  && (val = exact_log2 (UINTVAL (trueop1))) > 0)
> @@ -3154,12 +3186,17 @@ simplify_binary_operation_1 (enum rtx_co
>  	    }
>  	  /* x/1 is x.  */
>  	  if (trueop1 == CONST1_RTX (mode))
> -	    return rtl_hooks.gen_lowpart_no_emit (mode, op0);
> +	    {
> +	      tem = rtl_hooks.gen_lowpart_no_emit (mode, op0);
> +	      if (tem)
> +		return tem;
> +	    }
>  	  /* x/-1 is -x.  */
>  	  if (trueop1 == constm1_rtx)
>  	    {
>  	      rtx x = rtl_hooks.gen_lowpart_no_emit (mode, op0);
> -	      return simplify_gen_unary (NEG, mode, x, mode);
> +	      if (x)
> +		return simplify_gen_unary (NEG, mode, x, mode);
>  	    }
>  	}
>        break;
> --- gcc/dwarf2out.c.jj	2012-12-11 09:25:18.000000000 +0100
> +++ gcc/dwarf2out.c	2012-12-17 18:19:31.706376594 +0100
> @@ -11840,6 +11840,7 @@ mem_loc_descriptor (rtx rtl, enum machin
>    dw_loc_descr_ref mem_loc_result = NULL;
>    enum dwarf_location_atom op;
>    dw_loc_descr_ref op0, op1;
> +  rtx inner = NULL_RTX;
>  
>    if (mode == VOIDmode)
>      mode = GET_MODE (rtl);
> @@ -11869,35 +11870,39 @@ mem_loc_descriptor (rtx rtl, enum machin
>  	 contains the given subreg.  */
>        if (!subreg_lowpart_p (rtl))
>  	break;
> +      inner = SUBREG_REG (rtl);
> +    case TRUNCATE:
> +      if (inner == NULL_RTX)
> +        inner = XEXP (rtl, 0);
>        if (GET_MODE_CLASS (mode) == MODE_INT
> -	  && GET_MODE_CLASS (GET_MODE (SUBREG_REG (rtl))) == MODE_INT
> +	  && GET_MODE_CLASS (GET_MODE (inner)) == MODE_INT
>  	  && (GET_MODE_SIZE (mode) <= DWARF2_ADDR_SIZE
>  #ifdef POINTERS_EXTEND_UNSIGNED
>  	      || (mode == Pmode && mem_mode != VOIDmode)
>  #endif
>  	     )
> -	  && GET_MODE_SIZE (GET_MODE (SUBREG_REG (rtl))) <= DWARF2_ADDR_SIZE)
> +	  && GET_MODE_SIZE (GET_MODE (inner)) <= DWARF2_ADDR_SIZE)
>  	{
> -	  mem_loc_result = mem_loc_descriptor (SUBREG_REG (rtl),
> -					       GET_MODE (SUBREG_REG (rtl)),
> +	  mem_loc_result = mem_loc_descriptor (inner,
> +					       GET_MODE (inner),
>  					       mem_mode, initialized);
>  	  break;
>  	}
>        if (dwarf_strict)
>  	break;
> -      if (GET_MODE_SIZE (mode) > GET_MODE_SIZE (GET_MODE (SUBREG_REG (rtl))))
> +      if (GET_MODE_SIZE (mode) > GET_MODE_SIZE (GET_MODE (inner)))
>  	break;
> -      if (GET_MODE_SIZE (mode) != GET_MODE_SIZE (GET_MODE (SUBREG_REG (rtl)))
> +      if (GET_MODE_SIZE (mode) != GET_MODE_SIZE (GET_MODE (inner))
>  	  && (GET_MODE_CLASS (mode) != MODE_INT
> -	      || GET_MODE_CLASS (GET_MODE (SUBREG_REG (rtl))) != MODE_INT))
> +	      || GET_MODE_CLASS (GET_MODE (inner)) != MODE_INT))
>  	break;
>        else
>  	{
>  	  dw_die_ref type_die;
>  	  dw_loc_descr_ref cvt;
>  
> -	  mem_loc_result = mem_loc_descriptor (SUBREG_REG (rtl),
> -					       GET_MODE (SUBREG_REG (rtl)),
> +	  mem_loc_result = mem_loc_descriptor (inner,
> +					       GET_MODE (inner),
>  					       mem_mode, initialized);
>  	  if (mem_loc_result == NULL)
>  	    break;
> @@ -11909,7 +11914,7 @@ mem_loc_descriptor (rtx rtl, enum machin
>  	      break;
>  	    }
>  	  if (GET_MODE_SIZE (mode)
> -	      != GET_MODE_SIZE (GET_MODE (SUBREG_REG (rtl))))
> +	      != GET_MODE_SIZE (GET_MODE (inner)))
>  	    cvt = new_loc_descr (DW_OP_GNU_convert, 0, 0);
>  	  else
>  	    cvt = new_loc_descr (DW_OP_GNU_reinterpret, 0, 0);
> @@ -12666,7 +12671,6 @@ mem_loc_descriptor (rtx rtl, enum machin
>        break;
>  
>      case COMPARE:
> -    case TRUNCATE:
>        /* In theory, we could implement the above.  */
>        /* DWARF cannot represent the unsigned compare operations
>  	 natively.  */
> --- gcc/testsuite/g++.dg/opt/pr55717.C.jj	2012-12-17 19:27:04.160509897 +0100
> +++ gcc/testsuite/g++.dg/opt/pr55717.C	2012-12-17 19:26:36.000000000 +0100
> @@ -0,0 +1,107 @@
> +// PR debug/55717
> +// { dg-do compile }
> +// { dg-options "-O -g" }
> +
> +struct DebugOnly {};
> +template <class T>
> +struct StripConst { typedef T result; };
> +class TempAllocPolicy {};
> +template <class T>
> +class HashTableEntry
> +{
> +  unsigned keyHash;
> +  template <class, class, class>
> +  friend class HashTable;
> +  T t;
> +  void setLive (unsigned hn) { keyHash = hn; }
> +};
> +template <class T, class HashPolicy, class>
> +struct HashTable
> +{
> +  typedef typename HashPolicy::KeyType Key;
> +  typedef typename HashPolicy::Lookup Lookup;
> +  typedef HashTableEntry <T> Entry;
> +  struct Range
> +  {
> +    Range () {}
> +    Entry *cur, end;
> +    bool empty () { return false; }
> +    T front () { return T (); }
> +  };
> +  struct Enum : public Range
> +  {
> +    HashTable table;
> +    bool removed;
> +    template <class Map>
> +    Enum (Map map) : Range (map.all ()), table (map.impl), removed () {}
> +    void rekeyFront (Lookup l, Key)
> +    {
> +      T t = this->cur->t;
> +      table.putNewInfallible (l, t);
> +    }
> +    void rekeyFront (Key k)
> +    {
> +      rekeyFront (k, k);
> +    }
> +  };
> +  unsigned entryCount;
> +  unsigned sCollisionBit;
> +  unsigned prepareHash (Lookup l)
> +  {
> +    unsigned keyHash (HashPolicy::hash (l));
> +    return keyHash & sCollisionBit;
> +  }
> +  static Entry *entryp;
> +  Entry *findFreeEntry (unsigned) { return entryp; }
> +  void putNewInfallible (Lookup l, T)
> +  {
> +    unsigned keyHash = prepareHash (l);
> +    Entry *entry = findFreeEntry (keyHash);
> +    entry->setLive (keyHash);
> +    entryCount++;
> +  }
> +};
> +template <class Key>
> +struct HashMapEntry { Key key; };
> +template <class Key, class Value, class HashPolicy = DebugOnly, class AllocPolicy = TempAllocPolicy>
> +struct HashMap
> +{
> +  typedef HashMapEntry <Key> Entry;
> +  struct MapHashPolicy : HashPolicy
> +  {
> +    typedef Key KeyType;
> +  };
> +  typedef HashTable <Entry, MapHashPolicy, AllocPolicy> Impl;
> +  Impl impl;
> +  typedef typename Impl::Range Range;
> +  Range all () { return Range (); }
> +  typedef typename Impl::Enum Enum;
> +};
> +class FreeOp;
> +struct AllocationSiteKey;
> +typedef HashMap <AllocationSiteKey, DebugOnly, AllocationSiteKey, TempAllocPolicy> AllocationSiteTable;
> +struct TypeCompartment
> +{
> +  AllocationSiteTable *allocationSiteTable;
> +  void sweep (FreeOp *);
> +};
> +struct JSScript { unsigned *code; };
> +bool IsScriptMarked (JSScript **);
> +struct AllocationSiteKey
> +{
> +  JSScript *script;
> +  unsigned offset : 24;
> +  int kind;
> +  typedef AllocationSiteKey Lookup;
> +  static unsigned hash (AllocationSiteKey key) { return (long (key.script->code + key.offset)) ^ key.kind; }
> +};
> +void
> +TypeCompartment::sweep (FreeOp *)
> +{
> +  for (AllocationSiteTable::Enum e (*allocationSiteTable); !e.empty ();)
> +    {
> +      AllocationSiteKey key = e.front ().key;
> +      IsScriptMarked (&key.script);
> +      e.rekeyFront (key);
> +    }
> +}
> 
> 	Jakub
>