Re: [PATCH][1/3] Re-submission of Altera Nios II port, gcc parts

[Chung-Lin Tang <cltang at codesourcery dot com>]

On 13/11/20 1:34 AM, Richard Sandiford wrote:
> Chung-Lin Tang <cltang@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.

Will any RTL-level passes, say for example, propagate a zero-constant
into the pattern, without also simplifying it to a NOT pattern? (that's
a question, I don't really know)

Personally, I haven't really seen many cases optimizable to NOR, but I
think keeping the 'M' there is pretty harmless.

> 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:

The *norsi3 here is of course straightforward. I was referring to other
cases, like the AND/IOR/XOR pattern above, where I wanted to combine
them into a single alternative. That needs a bit more work to reorganize
the nios2_print_operand() cases.

> (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.

That will definitely work, though I don't think the zero case does any harm.

>>>> +;; 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".

For EQ/NE I guess that's the case, for the other comparisons I'm not so
sure; I'm not familiar enough with the details of the expander machinery
to claim anything.

If this doesn't carry to other comparisons, I intend to keep it in line
with the other cmp patterns. I experimented a bit with the generated
code, nothing is affected.

>>>> +      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.

It will work, and I did use it in some places, though I did not
exhaustively search-and-replace.

For (subtle) performance reasons, emit_move_insn() really does too much
as a backend utility. Usually backend code is already very precise on
what we want to generate.

>>>> +  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.)

I will change to use the over-the-top style in the final patch.

Thanks,
Chung-Lin