[rfc PATCH] rs6000: Updated constraint documentation

[Bill Schmidt]

On 1/31/20 9:42 AM, Segher Boessenkool wrote:
> Hi Bill,
>
> Thanks a lot for looking at this!  :-)
>
> On Fri, Jan 31, 2020 at 08:49:21AM -0600, Bill Schmidt wrote:
>>> +(define_register_constraint "wa"
>>> "rs6000_constraints[RS6000_CONSTRAINT_wa]"
>>> +  "A VSX register (VSR), @code{vs0}@dots{}@code{vs63}.  Either a @code{d}
>>> +   or a @code{v} register.")
>> Not quite true, as the "d" register is only half of a VSX register.  It
>> may or may not be worth including a picture of register overlaps...
> No, the "d" registers are the actual full registers, all 128 bits of it.
> You often use them in a mode that uses only 64 bits, sure.


Perhaps that would be worth a few words when describing the "d" 
constraint, then.  This is not at all obvious to the casual user. Thanks!

>
> I was planning to update this to
>
> (define_register_constraint "wa" "rs6000_constraints[RS6000_CONSTRAINT_wa]"
>    "A VSX register (VSR), @code{vs0}@dots{}@code{vs63}.  This is either an
>     FPR (@code{d}) or a VR (@code{v}).")
>
> Does that improve it?


Yes, sure.

>
> The numbering thing is also mentioned in the %x output modifier stuff.
> There must be a better way to present this, but I don't see it yet.  Hrm.


I honestly thought that was pretty good as is.

Thanks again!
Bill

>
>>>   (define_register_constraint "we"
>>>   "rs6000_constraints[RS6000_CONSTRAINT_we]"
>>> -  "VSX register if the -mpower9-vector -m64 options were used or
>>> NO_REGS.")
>>> +  "@internal VSX register if the -mpower9-vector -m64 options were used
>>> +   or NO_REGS.")
>> Suggest changing "used or" to "used, else".
> Or just "used."; this is internals documentation only, and all similar
> constraints will ideally go away at some point (it just didn't fit in
> easily with the "enabled" attribute yet; it probably should be just "p9"
> for "isa" and test the TARGET_64BIT in the insn condition, something
> like that.  Or maybe there shouldn't be separate handling for 64-bit
> at all here).
>
>>>   (define_register_constraint "wr"
>>>   "rs6000_constraints[RS6000_CONSTRAINT_wr]"
>>> -  "General purpose register if 64-bit instructions are enabled or
>>> NO_REGS.")
>>> +  "@internal General purpose register if 64-bit instructions are enabled
>>> +   or NO_REGS.")
>> Similar here.
> Yup.  I didn't change this, fwiw, just synched up md.texi and
> constraints.md where they diverged.
>
>>>   (define_memory_constraint "es"
>>> -  "A ``stable'' memory operand; that is, one which does not include any
>>> -automodification of the base register.  Unlike @samp{m}, this constraint
>>> -can be used in @code{asm} statements that might access the operand
>>> -several times, or that might not access it at all."
>>> +  "@internal
>>> +   A ``stable'' memory operand; that is, one which does not include any
>>> +   automodification of the base register.  This used to be useful when
>>> +   @code{m} allowed automodification of the base register, but as those
>> Trailing whitespace here.
> Yeah, I don't know how I missed that, git tends to shout about it.
> Fixed.
>
>>>   @item wa
>>> -Any VSX register if the @option{-mvsx} option was used or NO_REGS.
>>> +A VSX register (VSR), @code{vs0}@dots{}@code{vs63}.  Either a @code{d} or
>>> a @code{v}
>>> +register.
>> Same concern as above.
> It is literally the same text now (unless I messed up the c'n'p).
>
>>> +@ifset INTERNALS
>>> +@item h
>>> +@code{vrsave}, @code{ctr}, or @code{lr}.
>>> +@end ifset
>>
>> I don't see vrsave elsewhere in either document (should have noted this
>> in constraints.md also).
> There is no other constraint for vrsave.  constraints.md says
>
> (define_register_constraint "h" "SPECIAL_REGS"
>    "@internal @code{vrsave}, @code{ctr}, or @code{lr}.")
>
> (Same text, as should be).  It ends up only in gccint.*, not in gcc.* .
>
>>>   @item we
>>> -VSX register if the @option{-mcpu=power9} and @option{-m64} options
>>> -were used or NO_REGS.
>>> +VSX register if the -mpower9-vector -m64 options were used or NO_REGS.
>> As above.  I won't call out the rest of these.
> Since this is not new text, and it now only ends up in the internals
> documentation, and a lot of it should go away in the short term anyway,
> and importantly I don't know a good simple way to write what it does
> anyway (because it *isn't* simple), I hoped I could just keep this for
> now.
>
> Hrm, I lost markup there, will fix.
>
>>> +@item wZ
>>> +Indexed or indirect memory operand, ignoring the bottom 4 bits.
>>> +@end ifset
>> For consistency, "An indexed..." ?
> Yes, thanks!
>
>>> +@item Z
>>> +A memory operand that is an indexed or indirect from a register.
>> "indexed or indirect access"?
> And s/from a register// yeah.
>
>> Great improvements!
> Thanks :-)
>
> Somewhere it should say (in the gcc.* doc) that there are other
> constraints and output modifiers as well, and some are even supported
> for backwards compatibility, but here only the ones you should use are
> mentioned.  Not sure where to do that.
>
>
> Segher