[9] RFR(M): 8130832: Extend the WhiteBox API to provide information about the availability of compiler intrinsics (original) (raw)

Vladimir Kozlov vladimir.kozlov at oracle.com
Mon Jul 27 16:38:04 UTC 2015

Previous message: [9] RFR(M): 8130832: Extend the WhiteBox API to provide information about the availability of compiler intrinsics
Next message: [9] RFR(M): 8130832: Extend the WhiteBox API to provide information about the availability of compiler intrinsics
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

WB method isIntrinsicAvailableForMethod0 has parameter compilationContext. So I don't think you need is_intrinsic_available(methodHandle method) variant - it is not used from WB.

You left is_intrinsic_available*_for* in comments in abstractCompiler.hpp

Following the same naming logic I would suggest to remove "ForMethod" in WB new methods names.

Missing 'virtual' in c2compiler.hpp

Leftover line in c2compiler.cpp:

//return Compile::is_intrinsic_available(method, compilation_context, false);

Indention is off - keep following lines at the same level as !compilation_context (one space after "("):

```
  (!compilation_context.is_null() &&
```

      CompilerOracle::has_option_value(compilation_context,

"DisableIntrinsic", disable_intr) &&

      strstr(disable_intr, vmIntrinsics::name_at(id)) != NULL)

Remove 'return false' because following InlineUnsafeOps check disable some intrinsics:

```
case vmIntrinsics::_Reference_get:
```
```
  return false;
```
```
  break;
```

Also I would prefer vmIntrinsics::is_disabled_by_flags() was called from compiler's is_intrinsic_disabled_by_flag() flag.

Additionally I think we should not have difference in behavior of is_intrinsic_disabled_by_flag() in C1 and C2. Both compilers should follow the same rules. If we disable intrinsic on command line - C1 should not intrinsify it. The only difference should be in supported intrinsics. If you think it is big change - file an other bug (it is bug, not rfe) to fix it separately.

Thanks, Vladimir

On 7/22/15 12:59 AM, Zoltán Majó wrote:

Hi John,

On 07/21/2015 09:02 PM, John Rose wrote: Yes, that will work, and I think it is cleaner than what we had before, as well as providing the new required functionality.

Reviewed; please get a second reviewer. thank you for the review! I'll ask Vladimir K., maybe he has time to look at the newest webrev.

— John P.S. If the unit tests want to test (via the whitebox API) whether an intrinsic was compiled successfully, we might want to expose Compile::gatherintrinsicstatistics, etc. But not in this change set. That is an interesting idea. We'd also have to see if current tests require such functionality or if SQE plans to add tests requiring that functionality. P.P.S. As I think I said before, I wish we had a way to consolidate the switch statements further (into vmSymbols.hpp). But I don't see a clean way to do it. Yes, that would be nice. I've not seen a good way to do that, partly because inconsistencies between the way C1 and C2 depends on the value of command-line flags. Thank you! Best regards, Zoltan On Jul 21, 2015, at 8:19 AM, Zoltán Majó <zoltan.majo at oracle.com_ _<mailto:zoltan.majo at oracle.com>> wrote:

Here is the newest webrev: - top:http://cr.openjdk.java.net/~zmajo/8130832/top/ <http://cr.openjdk.java.net/%7Ezmajo/8130832/top/> - hotspot:http://cr.openjdk.java.net/~zmajo/8130832/hotspot/webrev.03/ <http://cr.openjdk.java.net/%7Ezmajo/8130832/hotspot/webrev.03/>

Previous message: [9] RFR(M): 8130832: Extend the WhiteBox API to provide information about the availability of compiler intrinsics
Next message: [9] RFR(M): 8130832: Extend the WhiteBox API to provide information about the availability of compiler intrinsics
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

More information about the hotspot-compiler-dev mailing list