On Mon, May 02, 2016 at 11:01:02PM +0200, Uros Bizjak wrote:
> > +(define_expand "atomic_bit_test_and_set<mode>"
> > + [(match_operand:SWI248 0 "register_operand")
> > + (match_operand:SWI248 1 "memory_operand")
> > + (match_operand:SWI248 2 "nonmemory_operand")
> > + (match_operand:SI 3 "const_int_operand") ;; model
> > + (match_operand:SI 4 "const_int_operand")]
> > + ""
> > +{
> > + emit_insn (gen_atomic_bit_test_and_set<mode>_1 (operands[1], operands[2],
> > + operands[3]));
> > + operands[5] = gen_reg_rtx (QImode);
>
> Please don't use operands[N] without corresponding (match_dup N) in
> the RTL pattern. Tthe "operands" array is only as long as the last
> operand number from the pattern. Just grep the pattern name from
> generated insn-emit.c and you will see the problem.
Ok, will add a temporary instead.
> > + [(set (reg:CCC FLAGS_REG)
> > + (compare:CCC
> > + (unspec_volatile:SWI248
> > + [(match_operand:SWI248 0 "memory_operand" "+m")
> > + (match_operand:SI 2 "const_int_operand")] ;; model
> > + UNSPECV_XCHG)
> > + (const_int 0)))
> > + (set (zero_extract:SWI248 (match_dup 0)
> > + (const_int 1)
> > + (match_operand:SWI248 1
> > + "nonmemory_operand" "<r>N"))
>
> No need for <r> attribute with SWI248, you can use plain "r" above.
> <r> makes difference only for QImode.
You're right.
> > --- gcc/testsuite/gcc.target/i386/pr49244-2.c.jj 2016-05-02
> > 12:51:51.501983254 +0200
> > +++ gcc/testsuite/gcc.target/i386/pr49244-2.c 2016-05-02
> > 14:47:30.240202019 +0200
> > @@ -0,0 +1,109 @@
> > +/* PR target/49244 */
> > +/* { dg-do run } */
> > +/* { dg-options "-O2 -g" } */
>
> Probably no need for -g in the testcase?
The -g in there is intentional, I had to write some debug info handling
code in the tree-ssa-ccp.c recognizer and at some point it ICEd.
> > +/* { dg-additional-options "-march=i486" { target ia32 } } */
>
> Hm, a "dg-do run" testcase with -march is kind of dangerous without a
> runtime test for requested architecture. OTOH, do we really need
> -march here?
I've been afraid that because of the missing atomic instructions to support
the builtins it wouldn't be recognized, but actually it is, and the
lock; bt[src] instructions are already i386+, so I'll drop that line.
That said, I think we already have some dg-do run tests with -march=i486
anyway, and don't really support libgomp, libitm etc. on pre-i486 (and glibc
doesn't support pre-i486 too).
Jakub
