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

[Richard Sandiford]

Chung-Lin Tang <cltang@codesourcery.com> writes:
> 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)

No, they shouldn't, because only the simplified form is expected to match.
E.g. forwprop goes to some lengths to do this properly.

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

That's not the way to look at it though.  There should be no
unsimplified rtl in the instruction stream, just like there should
be no non-canonical rtl.  We don't have rules for canonical rtl
because the other forms are "harmful" (in the sense of generating wrong
code or whatever).  We have them so that there aren't too many possible
representations of the same thing, and so that code dealing with existing
rtl patterns can validly expect one "shape" over another.  If we see
something that's generating the wrong rtl form, we should fix it rather
than pander to it.

IMO having (and (not (const_int 0)) (not X)) as an alternative
representation of (not X) falls directly into that category.

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

Sure, but the point is that EQ and NE _are_ commutative, and rtl.texi
says that:

  For commutative binary operations, constants should be placed in the
  second operand.

So...

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

...no, it doesn't carry over to other comparisons, but it's not supposed to.
Just like you can't add "%" to the other comparisons.

The patterns are including constraints whose only purpose is to match
non-canonical rtl.  That shouldn't happen.

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

Hmm, this sounds a bit FUDish.  What subtle reasons do you mean?

If moving a constant into a register is valid as a simple SET
then that's what emit_move_insn should do.

>>>>> +  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,
Richard