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