Thank you Jakub, I update the patch with your comments and tested it. Please let me know your thoughts/suggestions.
On Mon, Nov 19, 2018 at 4:00 PM Lokesh Janghel
<lokeshjanghe...@gmail.com> wrote:
>
> Thank you Jakub, I update the patch with your comments and tested it.
> Please let me know your thoughts/suggestions.
>
>
> Thanks
> Lokesh
>
> On Fri, Nov 16, 2018 at 4:57 PM Jakub Jelinek <ja...@redhat.com> wrote:
>>
>> On Fri, Nov 16, 2018 at 04:21:25PM +0530, Umesh Kalappa wrote:
>> > My bad ,
>> > attached the same now .
>>
>> +2018-11-15 Lokesh Janghel <lokeshjanghe...@gmail.com>
>>
>> Two spaces before < instead of just one.
>> +
>> + PR target/85667
>>
>> Only a single space between PR and target.
>>
>> + * i386.c (function_value_ms_64): ms_abi insist to use the eax
>>
>> The filename is relative to the directory with ChangeLog file, so
>> * config/i386/i386.c
>> in this case. The description should say what you've changed, so
>> something like:
>> * config/i386/i386.c (function_value_ms_64): Return AX_REG instead
>> of FIRST_SSE_REG for 4 or 8 byte modes.
>>
>> --- a/gcc/config/i386/i386.c
>> +++ b/gcc/config/i386/i386.c
>> @@ -9008,7 +9008,7 @@ function_value_ms_64 (machine_mode orig_mode,
>> machine_mode mode,
>> case 8:
>> case 4:
>> if (mode == SFmode || mode == DFmode)
>> - regno = FIRST_SSE_REG;
>> + regno = AX_REG;
>> break;
>>
>> Is there something to back that up, say godbolt.org link with some testcases
>> showing how does MSVC, clang etc. handle those?
>> And, because the function starts with:
>> unsigned int regno = AX_REG;
>> the change isn't right, you should remove all of:
>> case 8:
>> case 4:
>> if (mode == SFmode || mode == DFmode)
>> regno = FIRST_SSE_REG;
>> break;
>> because the default will do what you want.
>>
>> diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
>> index 50e53f0..ec54330 100644
>> --- a/gcc/testsuite/ChangeLog
>> +++ b/gcc/testsuite/ChangeLog
>> @@ -1,3 +1,8 @@
>> +2018-11-15 Lokesh Janghel <lokeshjanghe...@gmail.com>
>>
>> Two spaces between date and name.
>>
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.dg/pr85667.c
>> @@ -0,0 +1,29 @@
>> +/* { dg-options "-O2 " } */
>> +/* { dg-final { scan-assembler-times "movl\[\t
>> \]\.\[a-zA-Z\]\[a-zA-Z\]\[0-9\]\\(%rip\\), %eax" 1} } */
>>
>> First of all, the test is misplaced, it is clearly x86_64 specific
>> and probably lp64 only, so it shouldn't be in gcc.dg/ where it will
>> be run on all targets, but in gcc.target/i386/ and be guarded with
>> { target lp64 }.
>>
>> Second, seems like you'd like to run the testcase, so you'd better have it
>> /* { dg-do run { target lp64 } } */
>>
>> The assembler scanning will work only with -masm=att, not -masm=intel
>> and seems to be very fragile, so I'd suggest have one runtime test and one
>> compile time test in which you put just the fn1 function. Why two arbitrary
>> letters after dot? That makes on sense. Either you are looking for
>> .LC\[0-9]*
>> specifically, or for arbitrary symbol, then use something like
>> "movl\[^\n\r]*, %eax"
>> or so (and make sure to use -masm=intel).
>>
>> More interesting would be
>> make check ALT_CC_UNDER_TEST=msvc ALT_CXX_UNDER_TEST=msvc++
>> RUNTESTFLAGS='compat.exp struct-layout-1.exp'
>> (or whatever MSVC driver names are), i.e. try to run the compat testsuites
>> between MSVC and newly built gcc.
>>
>> Jakub
>
>
>
> --
> Thanks & Regards
> Lokesh Janghel
> +91-9752984749
--
Thanks & Regards
Lokesh Janghel
+91-9752984749
85667.patch
Description: Binary data
