Re: S/390: Fix warnings in "*setmem_long..." patterns.

["Ulrich Weigand" <uweigand@de.ibm.com>]

Andreas Krebbel wrote:
> On 11/30/2015 04:11 PM, Dominik Vogt wrote:
> > The attached patch fixes some warnings generated by the setmem...
> > patterns in s390.md during build and add test cases for the
> > patterns.  The patch is to be added on to p of the movstr patch:
> > https://gcc.gnu.org/ml/gcc-patches/2015-11/msg03485.html
> > 
> > The test cases validate that the patterns are actually used, but
> > at the moment the setmem_long_and pattern is never actually used
> > and thus the test case would fail.  So I've split the patch in two
> > (both attached to this message) to activate this part of the test
> > once we've fixed that.
> > 
> > The patch has passed the SPEC2006 testsuite without any measurable
> > changes in performance.
> 
> Shouldn't we instead describe the whole setmem operation as unspec including the other operands as
> well? The semantics of the introduced UNSPEC_P_TO_BLK operation is not clear to me.  It suggests to
> be some kind of "cast" which it isn't. In fact it is not able to do its job without the length which
> is specified as use outside the unspec.

Well, I guess I suggested to Dominik to leave the basic
[parallel
  (set (dst:BLK) (src:BLK))
  (use (length)]
structure in place; my understanding is that the middle-end recognizes
this as a block move.  As "source" in this case we'd use a BLKmode
operand that consist iof the same byte replicated a number of times.

If we were to use just a single UNSPEC, how would we indicate to the
middle-end that a block of memory is modified, without using too coarse-
grained clobbers?

However, I agree that UNSPEC_P_TO_BLK really should also get the length
as input, to make it have precisely defined semantics.  Also, I'd rather
use a more descriptive name, like UNSPEC_REPLICATE_BYTE or the like.

What would you think about something like the following?

(define_insn "*setmem_long"
  [(clobber (match_operand:<DBL> 0 "register_operand" "=d"))
   (set (mem:BLK (subreg:P (match_operand:<DBL> 3 "register_operand" "0") 0))
        (unspec:BLK [(match_operand:P 2 "shift_count_or_setmem_operand" "Y")
                     (subreg:P (match_dup 3) 1)] UNSPEC_REPLICATE_BYTE))
   (use (match_operand:<DBL> 1 "register_operand" "d"))
   (clobber (reg:CC CC_REGNUM))]

[ Not sure if we'd need an extra (use (match_dup 3)) any more. ]

B.t.w. this is certainly wrong and cannot be generated by common code:
        (and:BLK (unspec:BLK
	      [(match_operand:P 2 "shift_count_or_setmem_operand" "Y")]
	      UNSPEC_P_TO_BLK)
 	     (match_operand 4 "const_int_operand"             "n"))
(This explains why the pattern would never match.)

The AND should be on the filler byte instead:
        (unspec:BLK [(and:P (match_operand:P 2 "shift_count_or_setmem_operand" "Y")
 	                    (match_operand:P 4 "const_int_operand"             "n"))
                     (subreg:P (match_dup 3) 1)] UNSPEC_REPLICATE_BYTE))

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com