On 13/11/21 7:21 AM, Richard Sandiford wrote: > Chung-Lin Tang <clt...@codesourcery.com> writes: >> 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) > > 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.
I'm not saying we tolerate "wrong" RTL form, but rather that, it should be an issue of the RTL passes, not the backend. The backend should just be as much as possible, a "machine description". Saying you don't need to describe a particular thing in the backend because "GCC doesn't need that" sounds like a spill-over of internal details. In theory, a backend writer shouldn't need to care for such things. >>>>>> +;; 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. My argument is the same as above. Having that said, I'll edit the *norsi3 and EQNE patterns to remove the 'M' like you suggested in the final patch. This is too trivial an issue to get postponed by. >>>>>> + 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. "FUDish" sounds too serious. I'm just saying that, when a backend emits a constant/const-address move (unlike the machine-independent code which is abstracted away from target details) it probably doesn't intend to have it also passed through the constant/address legitmat(e_p|ize) hooks. All I want is a simple SET generated. Thanks, Chung-Lin >>>>>> + 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 >
