Re: [PATCH 3/9] Add option -moutline-msabi-xlogues

[Daniel Santos <daniel.santos@pobox.com>]

Thank you very much for your review!

On 11/25/2016 05:51 PM, Sandra Loosemore wrote:
> On 11/22/2016 10:19 PM, Daniel Santos wrote:
>
>> diff --git a/gcc/config/i386/i386.opt b/gcc/config/i386/i386.opt
>> index 9eef558..f556978 100644
>> --- a/gcc/config/i386/i386.opt
>> +++ b/gcc/config/i386/i386.opt
>> @@ -528,6 +528,11 @@ Enum(calling_abi) String(sysv) Value(SYSV_ABI)
>>   EnumValue
>>   Enum(calling_abi) String(ms) Value(MS_ABI)
>>
>> +moutline-msabi-xlogues
>> +Target Report Mask(OUTLINE_MSABI_XLOGUES) Save
>> +Reduces function size by using out-of-line stubs to save & restore 
>> registers
>> +clobberd by differences in Microsoft and System V ABIs.
>> +
>
> Just as a suggestion (I'm not an i386 maintainer), I'd recommend 
> spelling the name of this option -mno-inline-msabi-xlogues instead of 
> -moutline-msabi-xlogues, and making the default -minline-msabi-xlogues.

That is certainly more intuitive ("outline" or "out-of-line" vs 
"inline"). I guess I'm not sure what concept is more important, that 
it's not inline or that it's using stubs. Probably the former.

>
>> diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
>> index 8e2f466..4706085 100644
>> --- a/gcc/doc/invoke.texi
>> +++ b/gcc/doc/invoke.texi
>> @@ -25004,6 +25004,15 @@ You can control this behavior for specific 
>> functions by
>>   using the function attributes @code{ms_abi} and @code{sysv_abi}.
>>   @xref{Function Attributes}.
>>
>> +@item -moutline-msabi-xlogues
>> +@itemx -mno-outline-msabi-xlogues
>
> By convention, we only list the option form that is not the default....

OK. This is my first time to work on gcc, so still learning all of the 
conventions.

>
>> +@opindex moutline-msabi-xlogues
>
> ...but we should have index entries for both.

So basically I got that backwards. :)

>
>> +Due to differences in 64-bit ABIs, any Microsoft ABI function that 
>> calls a
>> +SysV ABI function must consider RSI, RDI and XMM6-15 as clobbered, 
>> emitting
>> +fairly lengthy prologues & epilogues.  This option generates 
>> prologues &
>> +epilogues that instead call stubs in libgcc to perform these saves & 
>> restores,
>> +thus reducing function size at the cost of a few extra instructions.
>
> Please use the word "and" in all three locations here, instead of "&".
>
> -Sandra

Now that I think of it, would "System V" be better than just "SysV"?

This is very helpful, thanks again. I'm working on a meaningful 
testsuite, so I suppose I should just submit a v3 with these changes 
when I'm done with the tests and hopefully I'll have a little more 
feedback on the back end & assembly as well.

Daniel