Chung-Lin Tang <clt...@codesourcery.com> writes:
>>> +;;  Integer logical Operations
>>> +
>>> +(define_code_iterator LOGICAL [and ior xor])
>>> +(define_code_attr logical_asm [(and "and") (ior "or") (xor "xor")])
>>> +
>>> +(define_insn "<code>si3"
>>> +  [(set (match_operand:SI 0 "register_operand"             "=r,r,r")
>>> +        (LOGICAL:SI (match_operand:SI 1 "register_operand" "%r,r,r")
>>> +                    (match_operand:SI 2 "logical_operand"  "rM,J,K")))]
>>> +  ""
>>> +  "@
>>> +    <logical_asm>\\t%0, %1, %z2
>>> +    <logical_asm>%i2\\t%0, %1, %2
>>> +    <logical_asm>h%i2\\t%0, %1, %U2"
>>> +  [(set_attr "type" "alu")])
>>> +
>>> +(define_insn "*norsi3"
>>> +  [(set (match_operand:SI 0 "register_operand"                  "=r")
>>> +        (and:SI (not:SI (match_operand:SI 1 "register_operand"  "%r"))
>>> +                (not:SI (match_operand:SI 2 "reg_or_0_operand"  "rM"))))]
>>> +  ""
>>> +  "nor\\t%0, %1, %z2"
>>> +  [(set_attr "type" "alu")])
>> 
>> M constraints (for const0_rtx) and reg_or_0 seem unnecessary, no such
>> RTL should make it to this point.
>
> Such RTL does appear under -O0. Removing the 'M' will also
> require a bit of re-working the operand printing mechanics; not a lot of
> work, but I'd rather keep it as is. The behavior of using the zero
> register for a 0-value is also more expected in nios2, I think.

Hmm, but if we get (not (const_int 0)) then that sounds like a bug,
since (and (not X) (not (const_int 0))) should reduce to (not X).
IMO target-independent code shouldn't try to create the nor-with-0
form and backends shouldn't match it.

Why would removing 'M' affect the printing mechanism?  Naively I'd
have expected:

(define_insn "*norsi3"
  [(set (match_operand:SI 0 "register_operand"                  "=r")
        (and:SI (not:SI (match_operand:SI 1 "register_operand"  "r"))
                (not:SI (match_operand:SI 2 "register_operand"  "r"))))]
  ""
  "nor\\t%0, %1, %2"
  [(set_attr "type" "alu")])

to just work.

>>> +;; Integer comparisons
>>> +
>>> +(define_code_iterator EQNE [eq ne])
>>> +(define_insn "nios2_cmp<code>"
>>> +  [(set (match_operand:SI 0 "register_operand"           "=r")
>>> +        (EQNE:SI (match_operand:SI 1 "reg_or_0_operand" "%rM")
>>> +                 (match_operand:SI 2 "arith_operand"     "rI")))]
>>> +  ""
>>> +  "cmp<code>%i2\\t%0, %z1, %z2"
>>> +  [(set_attr "type" "alu")])
>> 
>> Once again, using reg_or_0 and "M" seems pointless.
>
> The compares don't support all operations, with only the second operand
> capable of an immediate. Using 'rM' should theoretically allow more
> commutative swapping.

But rtl-wise, we should never generate an EQ or NE with two constants.
And if one operand is constant then it's supposed to be the second.

The "%" should give commutativity on its own, without the "M".

>>> +      emit_insn
>>> +   (gen_rtx_SET (Pmode, tmp,
>>> +                 gen_int_mode (cfun->machine->save_regs_offset,
>>> +                               Pmode)));
>> 
>> Shouldn't have a mode on the SET, but really should just call
>> emit_move_insn. Similar cases elsewhere, search for "gen_rtx_SET (Pmode".
>
> I've removed the modes on SET, though I prefer the more bare generation
> of the insns in some contexts; emit_move_insn() seems to have a lot
> under the hood.

There shouldn't be anything to be afraid of though.  Target-independent
code would use emit_move_insn for this though, so it needs to just work.

>>> +  HOST_WIDE_INT var_size;       /* # of var. bytes allocated.  */
>> 
>> Not the first time they occur in this file, but I suppose I should
>> mention that these in-line comments are probably just outside our coding
>> guidelines.
>
> Deleted the comments outside the struct defs.

FWIW, I think it was more that comments should be above the field rather
than tagged on the right.  (One of the big problems with right-column comments
is that people tend to make them too short.)

Thanks,
Richard

Reply via email to