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

Zoltán Majó zoltan.majo at oracle.com
Tue Jul 28 15:05:30 UTC 2015

Thank you, Vladimir, for the review!

Best regards,

Zoltan

On 07/28/2015 04:12 PM, Vladimir Kozlov wrote:

This looks good.

Thanks, Vladimir On 7/28/15 3:17 AM, Zoltán Majó wrote: Hi Vladimir,

thank you for the feedback! On 07/27/2015 06:38 PM, Vladimir Kozlov wrote: WB method isIntrinsicAvailableForMethod0 has parameter compilationContext. So I don't think you need isintrinsicavailable(methodHandle method) variant - it is not used from WB. You're right, I removed that variant. You left isintrinsicavailablefor in comments in abstractCompiler.hpp Updated. I also updated the text in the comments at other places so that they are more accurate than before.

Following the same naming logic I would suggest to remove "ForMethod" in WB new methods names. Updated the method names as well. Missing 'virtual' in c2compiler.hpp Updated as well. Leftover line in c2compiler.cpp: + //return Compile::isintrinsicavailable(method, compilationcontext, false); Thank you for spotting that, removed it. Indention is off - keep following lines at the same level as !compilationcontext (one space after "("): + (!compilationcontext.isnull() && + CompilerOracle::hasoptionvalue(compilationcontext, "DisableIntrinsic", disableintr) && + strstr(disableintr, vmIntrinsics::nameat(id)) != NULL) Updated the indentation. Remove 'return false' because following InlineUnsafeOps check disable some intrinsics: + case vmIntrinsics::Referenceget: + return false; + break; Oh, I missed that! Updated. Also I would prefer vmIntrinsics::isdisabledbyflags() was called from compiler's isintrinsicdisabledbyflag() flag. I moved the call to the compiler-specific isintrinsicdisabledbyflag methods. Additionally I think we should not have difference in behavior of isintrinsicdisabledbyflag() 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. Yes, I also think it makes sense that flags behave the same way for all compilers. But I would like to keep that as a separate issue, partly because I haven't figured out all details yet for implementing a fix and partly because the current issue is related to some "critical" nightly failures we have. So I filed JDK-8132457: "Unify command-line flags controlling the usage of compiler intrinsics" for addressing inconsistencies in processing intrinsic-related command-line flags. I would like to push the newest webrev (if you are fine with it) and then continue with JDK-8132457. I hope that is OK. Here is the newest webrev: - top: http://cr.openjdk.java.net/~zmajo/8130832/top/webrev.04/ - hotspot: http://cr.openjdk.java.net/~zmajo/8130832/hotspot/webrev.04/ Testing: - JPRT (testset "hotspot" + tests in compiler/intrinsics/mathexact); all tests pass. Thank you and best regards, Zoltan

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/>

More information about the hotspot-compiler-dev mailing list