https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87626

[Lokesh Janghel]

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
<lokeshjanghel91@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 <jakub@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 <lokeshjanghel91@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  <lokeshjanghel91@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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 85667.patch
Type: application/octet-stream
Size: 2448 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20181119/a4ee3d91/attachment.obj>