Hi Eric,
On Fri, Oct 02, 2020 at 10:26:24AM +0200, Eric Botcazou wrote:
> > Don't call it "mask" please: *all* of our basic rotate instructions
> > already have something called "mask" (that is the "m" in "rlwnm" for
> > example; and "rotlw d,a,b" is just an extended mnemonic for
> > "rlwnm d,a,b,0,31"). The hardware also does not mask the shift amount
> > at all (instead, only the low 5 bits of RB *are* the shift amount).
>
> "Mask" is a common terminology though, used e.g. in the x86 back-end. That
> being said, I agree that it conflicts with PowerPC specific stuff so please
> suggest an alternative here.
Something that describes what it does well ("mask" does *not* in our
context). "_shiftmask" maybe? I'm sure you can think of something
better than that ;-)
> > > +(define_insn "*rotl<mode>3_mask"
> > > + [(set (match_operand:GPR 0 "gpc_reg_operand" "=r")
> > > + (rotate:GPR (match_operand:GPR 1 "gpc_reg_operand" "r")
> > > + (and:GPR (match_operand:GPR 2 "gpc_reg_operand" "r")
> > > + (match_operand:GPR 3 "const_int_operand"
> "n"))))]
> > > + "(UINTVAL (operands[3]) & (GET_MODE_BITSIZE (<MODE>mode) - 1))
> > > + == (unsigned HOST_WIDE_INT) (GET_MODE_BITSIZE (<MODE>mode) - 1)"
> > Don't mask operands[3] please (in the UINTVAL): RTL with the number
> > outside of that range is *undefined*. So just check that it is equal?
>
> Copied from the x86 back-end as well, so I'd rather keep it that way, unless
> you really insist...
Improve the x86 backend instead, if you want to have the two backends
similar?
> > Why? This diverges the "dot" version from the non-dot for no reason.
>
> OK, let's drop it.
Some rationale: almost all dot and dot2 patterns are exactly the same as
the non-dot versions (they can be generated by a script even), and just
the insn condition makes the insn only valid for Pmode (for the insns
where that is true! Some SImode dot patterns are valid in 64-bit mode
as well!) This way is more obvious, more explicit.
> > I don't see a patch with splitters only? Huh. Did you attach the same
> > patch twice?
>
> AFAICS no, 2 different patches were attached.
Yes I see, but that uses define_insn(_and_split) for these still.
> > Since this won't be handled before combine (or what do I miss?), it is
> > fine to do splitters only (splitters for combine). But the other
> > approach is fine as well.
>
> Patch #2 uses define_and_split like the x86 back-end; moreover, we already
> have define_and_split for the dot variants so maybe it's the best way to go?
Please just use define_split, or just define_insn. define_split if you
want this to be split during combine (how could that work here though?),
or define_insn if you think other passes can generate this as well.
define_insn_and_split is *both*, and that isn't useful here.
Do you see dot patterns for this used ever, btw?
Segher
