RFR(S): 8179618: Fixes for range of OptoLoopAlignment and Inlining flags

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR(S): 8179618: Fixes for range of OptoLoopAlignment and Inlining flags

Lindenmaier, Goetz

Hi,

 

This change fixes range handling of a few flags of C2.

This should go to jdk10, and later be downported to some

update of jdk9.

 

Please review this change. I please need a sponsor.

http://cr.openjdk.java.net/~goetz/wr17/8179618-FlagRanges/webrev.01/

 

Class WarmCallInfo limits its values to 1.0e10, but the flags used

to set it's fields (HotCallCountThreshold etc.) are limited by max_intx.

Using values over 1.0e10 causes assertions in the debug build.

 

OptoLoopAlignment must be a multiple of nop size, else it's not

possible to generate the instructions that go into the pad.

On x86 NOP size is 1, so it's no problem.

For SPARC, OptoLoopAlignmentConstraintFunc implements a special

case for bigger NOPs. This is also needed for s390 and ppc.

I just removed the #define, as the code works also on platforms

where NOPsize == 1. Actually, it should be optimized by the C

compiler in these cases.

 

Best regards,

  Goetz.

Reply | Threaded
Open this post in threaded view
|

Re: RFR(S): 8179618: Fixes for range of OptoLoopAlignment and Inlining flags

Vladimir Kozlov
Looks good to me.

Thanks,
Vladimir

On 5/4/17 3:57 AM, Lindenmaier, Goetz wrote:

> Hi,
>
>
>
> This change fixes range handling of a few flags of C2.
>
> This should go to jdk10, and later be downported to some
>
> update of jdk9.
>
>
>
> Please review this change. I please need a sponsor.
>
> http://cr.openjdk.java.net/~goetz/wr17/8179618-FlagRanges/webrev.01/
>
>
>
> Class WarmCallInfo limits its values to 1.0e10, but the flags used
>
> to set it's fields (HotCallCountThreshold etc.) are limited by max_intx.
>
> Using values over 1.0e10 causes assertions in the debug build.
>
>
>
> OptoLoopAlignment must be a multiple of nop size, else it's not
>
> possible to generate the instructions that go into the pad.
>
> On x86 NOP size is 1, so it's no problem.
>
> For SPARC, OptoLoopAlignmentConstraintFunc implements a special
>
> case for bigger NOPs. This is also needed for s390 and ppc.
>
> I just removed the #define, as the code works also on platforms
>
> where NOPsize == 1. Actually, it should be optimized by the C
>
> compiler in these cases.
>
>
>
> Best regards,
>
>   Goetz.
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR(S): 8179618: Fixes for range of OptoLoopAlignment and Inlining flags

Thomas Stüfe-2
In reply to this post by Lindenmaier, Goetz

Hi Goetz,

c2_globals.hpp:

-          range(0, max_intx)                                                \
+          range(0, ((intx)MIN2((int64_t)max_intx,(int64_t)(+1.0e10))))      \

32bit: I would have expected a build warning for the cast. Is it okay that we can never reach the max value on 32bit?

You could probably loose some brackets (around +1.0e10 and around the whole MIN2 expression).

commandLineFlagConstraintsCompiler.cpp:

     CommandLineError::print(verbose,
                             "OptoLoopAlignment (" INTX_FORMAT ") must be "
                             "multiple of NOP size\n");

There is an error here, the print parameter is missing. Would have expected the compiler to complain, actually - at least the gcc. Again, curious.

Kind Regards, Thomas

On Thu, May 4, 2017 at 12:57 PM, Lindenmaier, Goetz <[hidden email]> wrote:

Hi,

 

This change fixes range handling of a few flags of C2.

This should go to jdk10, and later be downported to some

update of jdk9.

 

Please review this change. I please need a sponsor.

http://cr.openjdk.java.net/~goetz/wr17/8179618-FlagRanges/webrev.01/

 

Class WarmCallInfo limits its values to 1.0e10, but the flags used

to set it's fields (HotCallCountThreshold etc.) are limited by max_intx.

Using values over 1.0e10 causes assertions in the debug build.

 

OptoLoopAlignment must be a multiple of nop size, else it's not

possible to generate the instructions that go into the pad.

On x86 NOP size is 1, so it's no problem.

For SPARC, OptoLoopAlignmentConstraintFunc implements a special

case for bigger NOPs. This is also needed for s390 and ppc.

I just removed the #define, as the code works also on platforms

where NOPsize == 1. Actually, it should be optimized by the C

compiler in these cases.

 

Best regards,

  Goetz.


Reply | Threaded
Open this post in threaded view
|

RE: RFR(S): 8179618: Fixes for range of OptoLoopAlignment and Inlining flags

Lindenmaier, Goetz
Hi Thomas,

thanks for looking at my change.
New webrev:
http://cr.openjdk.java.net/~goetz/wr17/8179618-FlagRanges/webrev.02/

> c2_globals.hpp:
> -          range(0, max_intx)                                                \
> +          range(0, ((intx)MIN2((int64_t)max_intx,(int64_t)(+1.0e10))))      \
> 32bit: I would have expected a build warning for the cast. Is it okay that we can never reach the max value on 32bit?

I double checked that there is no warning in our night builds and on linuxintel.

> commandLineFlagConstraintsCompiler.cpp:
>      CommandLineError::print(verbose,
>                              "OptoLoopAlignment (" INTX_FORMAT ") must be "
>                              "multiple of NOP size\n");
> There is an error here, the print parameter is missing. Would have expected the compiler to complain, actually - at least the gcc. Again, curious.

Thanks, good catch! The error was there before, but fixed anyways. I also
added the NOP size.

Best regards,
  Goetz.



Kind Regards, Thomas

On Thu, May 4, 2017 at 12:57 PM, Lindenmaier, Goetz <mailto:[hidden email]> wrote:
Hi,
 
This change fixes range handling of a few flags of C2.
This should go to jdk10, and later be downported to some
update of jdk9.
 
Please review this change. I please need a sponsor.
http://cr.openjdk.java.net/~goetz/wr17/8179618-FlagRanges/webrev.01/
 
Class WarmCallInfo limits its values to 1.0e10, but the flags used
to set it's fields (HotCallCountThreshold etc.) are limited by max_intx.
Using values over 1.0e10 causes assertions in the debug build.
 
OptoLoopAlignment must be a multiple of nop size, else it's not
possible to generate the instructions that go into the pad.
On x86 NOP size is 1, so it's no problem.
For SPARC, OptoLoopAlignmentConstraintFunc implements a special
case for bigger NOPs. This is also needed for s390 and ppc.
I just removed the #define, as the code works also on platforms
where NOPsize == 1. Actually, it should be optimized by the C
compiler in these cases.
 
Best regards,
  Goetz.

Reply | Threaded
Open this post in threaded view
|

Re: RFR(S): 8179618: Fixes for range of OptoLoopAlignment and Inlining flags

Thomas Stüfe-2
Hi Goetz,

On Tue, May 9, 2017 at 4:17 PM, Lindenmaier, Goetz <[hidden email]> wrote:
Hi Thomas,

thanks for looking at my change.
New webrev:
http://cr.openjdk.java.net/~goetz/wr17/8179618-FlagRanges/webrev.02/

> c2_globals.hpp:
> -          range(0, max_intx)                                                \
> +          range(0, ((intx)MIN2((int64_t)max_intx,(int64_t)(+1.0e10))))      \
> 32bit: I would have expected a build warning for the cast. Is it okay that we can never reach the max value on 32bit?

I double checked that there is no warning in our night builds and on linuxintel.

> commandLineFlagConstraintsCompiler.cpp:
>      CommandLineError::print(verbose,
>                              "OptoLoopAlignment (" INTX_FORMAT ") must be "
>                              "multiple of NOP size\n");
> There is an error here, the print parameter is missing. Would have expected the compiler to complain, actually - at least the gcc. Again, curious.

Thanks, good catch! The error was there before, but fixed anyways. I also
added the NOP size.


+  // Relevant on ppc, s390, sparc. Will be optimized where
+  // addr_unit() == 1.
   if (OptoLoopAlignment % relocInfo::addr_unit() != 0) {
     CommandLineError::print(verbose,
                             "OptoLoopAlignment (" INTX_FORMAT ") must be "
-                            "multiple of NOP size\n");
+                            "multiple of NOP size (" INTX_FORMAT ")\n",
+                            value, relocInfo::addr_unit());

We are getting there...

addr_unit() returns int, so use %d, not INTX_FORMAT.

Apart from that all is fine. No need for a new webrev.

..Thomas


 
Best regards,
  Goetz.



Kind Regards, Thomas

On Thu, May 4, 2017 at 12:57 PM, Lindenmaier, Goetz <mailto:[hidden email]> wrote:
Hi,
 
This change fixes range handling of a few flags of C2.
This should go to jdk10, and later be downported to some
update of jdk9.
 
Please review this change. I please need a sponsor.
http://cr.openjdk.java.net/~goetz/wr17/8179618-FlagRanges/webrev.01/
 
Class WarmCallInfo limits its values to 1.0e10, but the flags used
to set it's fields (HotCallCountThreshold etc.) are limited by max_intx.
Using values over 1.0e10 causes assertions in the debug build.
 
OptoLoopAlignment must be a multiple of nop size, else it's not
possible to generate the instructions that go into the pad.
On x86 NOP size is 1, so it's no problem.
For SPARC, OptoLoopAlignmentConstraintFunc implements a special
case for bigger NOPs. This is also needed for s390 and ppc.
I just removed the #define, as the code works also on platforms
where NOPsize == 1. Actually, it should be optimized by the C
compiler in these cases.
 
Best regards,
  Goetz.


Reply | Threaded
Open this post in threaded view
|

RE: RFR(S): 8179618: Fixes for range of OptoLoopAlignment and Inlining flags

Lindenmaier, Goetz

Hi,

 

could someone please sponsor? Thanks!

 

I fixed the print statement.  New webrev anyways:

http://cr.openjdk.java.net/~goetz/wr17/8179618-FlagRanges/webrev.03/

 

Best regards,

  Goetz.

 

From: Thomas Stüfe [mailto:[hidden email]]
Sent: Tuesday, May 09, 2017 7:54 PM
To: Lindenmaier, Goetz <[hidden email]>
Cc: [hidden email]
Subject: Re: RFR(S): 8179618: Fixes for range of OptoLoopAlignment and Inlining flags

 

Hi Goetz,

 

On Tue, May 9, 2017 at 4:17 PM, Lindenmaier, Goetz <[hidden email]> wrote:

Hi Thomas,

thanks for looking at my change.
New webrev:
http://cr.openjdk.java.net/~goetz/wr17/8179618-FlagRanges/webrev.02/

> c2_globals.hpp:
> -          range(0, max_intx)                                                \
> +          range(0, ((intx)MIN2((int64_t)max_intx,(int64_t)(+1.0e10))))      \
> 32bit: I would have expected a build warning for the cast. Is it okay that we can never reach the max value on 32bit?

I double checked that there is no warning in our night builds and on linuxintel.

> commandLineFlagConstraintsCompiler.cpp:
>      CommandLineError::print(verbose,
>                              "OptoLoopAlignment (" INTX_FORMAT ") must be "
>                              "multiple of NOP size\n");
> There is an error here, the print parameter is missing. Would have expected the compiler to complain, actually - at least the gcc. Again, curious.

Thanks, good catch! The error was there before, but fixed anyways. I also
added the NOP size.


+  // Relevant on ppc, s390, sparc. Will be optimized where
+  // addr_unit() == 1.
   if (OptoLoopAlignment % relocInfo::addr_unit() != 0) {
     CommandLineError::print(verbose,
                             "OptoLoopAlignment (" INTX_FORMAT ") must be "
-                            "multiple of NOP size\n");
+                            "multiple of NOP size (" INTX_FORMAT ")\n",
+                            value, relocInfo::addr_unit());

We are getting there...

 

addr_unit() returns int, so use %d, not INTX_FORMAT.

 

Apart from that all is fine. No need for a new webrev.

 

..Thomas


 

Best regards,
  Goetz.



Kind Regards, Thomas


On Thu, May 4, 2017 at 12:57 PM, Lindenmaier, Goetz <mailto:[hidden email]> wrote:
Hi,
 
This change fixes range handling of a few flags of C2.
This should go to jdk10, and later be downported to some
update of jdk9.
 
Please review this change. I please need a sponsor.
http://cr.openjdk.java.net/~goetz/wr17/8179618-FlagRanges/webrev.01/
 
Class WarmCallInfo limits its values to 1.0e10, but the flags used
to set it's fields (HotCallCountThreshold etc.) are limited by max_intx.
Using values over 1.0e10 causes assertions in the debug build.
 
OptoLoopAlignment must be a multiple of nop size, else it's not
possible to generate the instructions that go into the pad.
On x86 NOP size is 1, so it's no problem.
For SPARC, OptoLoopAlignmentConstraintFunc implements a special
case for bigger NOPs. This is also needed for s390 and ppc.
I just removed the #define, as the code works also on platforms
where NOPsize == 1. Actually, it should be optimized by the C
compiler in these cases.
 
Best regards,
  Goetz.

 

Reply | Threaded
Open this post in threaded view
|

RE: RFR(S): 8179618: Fixes for range of OptoLoopAlignment and Inlining flags

Lindenmaier, Goetz
Hi,

could someone please sponsor this change?
Final webrev:
http://cr.openjdk.java.net/~goetz/wr17/8179618-FlagRanges/webrev.03/

Thanks,
  Goetz

> -----Original Message-----
> From: Lindenmaier, Goetz
> Sent: Freitag, 12. Mai 2017 09:10
> To: 'Thomas Stüfe' <[hidden email]>
> Cc: [hidden email]
> Subject: RE: RFR(S): 8179618: Fixes for range of OptoLoopAlignment and Inlining
> flags
>
> Hi,
>
>
>
> could someone please sponsor? Thanks!
>
>
>
> I fixed the print statement.  New webrev anyways:
>
> http://cr.openjdk.java.net/~goetz/wr17/8179618-FlagRanges/webrev.03/
>
>
>
> Best regards,
>
>   Goetz.
>
>
>
> From: Thomas Stüfe [mailto:[hidden email]]
> Sent: Tuesday, May 09, 2017 7:54 PM
> To: Lindenmaier, Goetz <[hidden email]>
> Cc: [hidden email]
> Subject: Re: RFR(S): 8179618: Fixes for range of OptoLoopAlignment and Inlining
> flags
>
>
>
> Hi Goetz,
>
>
>
> On Tue, May 9, 2017 at 4:17 PM, Lindenmaier, Goetz
> <[hidden email] <mailto:[hidden email]> > wrote:
>
> Hi Thomas,
>
> thanks for looking at my change.
> New webrev:
> http://cr.openjdk.java.net/~goetz/wr17/8179618-
> FlagRanges/webrev.02/
>
> > c2_globals.hpp:
> > -          range(0, max_intx)                                                \
> > +          range(0, ((intx)MIN2((int64_t)max_intx,(int64_t)(+1.0e10))))      \
> > 32bit: I would have expected a build warning for the cast. Is it okay
> that we can never reach the max value on 32bit?
>
> I double checked that there is no warning in our night builds and on
> linuxintel.
>
> > commandLineFlagConstraintsCompiler.cpp:
> >      CommandLineError::print(verbose,
> >                              "OptoLoopAlignment (" INTX_FORMAT ") must be "
> >                              "multiple of NOP size\n");
> > There is an error here, the print parameter is missing. Would have
> expected the compiler to complain, actually - at least the gcc. Again, curious.
>
> Thanks, good catch! The error was there before, but fixed anyways. I
> also
> added the NOP size.
>
>
> +  // Relevant on ppc, s390, sparc. Will be optimized where
> +  // addr_unit() == 1.
>    if (OptoLoopAlignment % relocInfo::addr_unit() != 0) {
>      CommandLineError::print(verbose,
>                              "OptoLoopAlignment (" INTX_FORMAT ") must be "
> -                            "multiple of NOP size\n");
> +                            "multiple of NOP size (" INTX_FORMAT ")\n",
> +                            value, relocInfo::addr_unit());
>
> We are getting there...
>
>
>
> addr_unit() returns int, so use %d, not INTX_FORMAT.
>
>
>
> Apart from that all is fine. No need for a new webrev.
>
>
>
> ..Thomas
>
>
>
>
> Best regards,
>  Goetz.
>
>
>
> Kind Regards, Thomas
>
>
> On Thu, May 4, 2017 at 12:57 PM, Lindenmaier, Goetz
> <mailto:[hidden email] <mailto:[hidden email]> >
> wrote:
> Hi,
>
> This change fixes range handling of a few flags of C2.
> This should go to jdk10, and later be downported to some
> update of jdk9.
>
> Please review this change. I please need a sponsor.
> http://cr.openjdk.java.net/~goetz/wr17/8179618-
> FlagRanges/webrev.01/
>
> Class WarmCallInfo limits its values to 1.0e10, but the flags used
> to set it's fields (HotCallCountThreshold etc.) are limited by max_intx.
> Using values over 1.0e10 causes assertions in the debug build.
>
> OptoLoopAlignment must be a multiple of nop size, else it's not
> possible to generate the instructions that go into the pad.
> On x86 NOP size is 1, so it's no problem.
> For SPARC, OptoLoopAlignmentConstraintFunc implements a special
> case for bigger NOPs. This is also needed for s390 and ppc.
> I just removed the #define, as the code works also on platforms
> where NOPsize == 1. Actually, it should be optimized by the C
> compiler in these cases.
>
> Best regards,
>  Goetz.
>
>