Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve conceptual integrity of SVM nomenclature #14582

Merged
merged 11 commits into from
Mar 7, 2022

Conversation

dsouzai
Copy link
Contributor

@dsouzai dsouzai commented Feb 23, 2022

The SVM, at its core, maintains a mapping between the results of front end VM queries and uint16_t IDs. Conceptually, the ID is the symbol that is associated with the result which is the value. However, in the SVM, the term "symbol" was used to refer to the value. This is confusing for anyone new to the SVM, especially since it claims to be similar to a symbol table in a linker.

This PR improves the conceptual integrity of the SVM by actually referring to values as values and IDs as symbols.

@dsouzai
Copy link
Contributor Author

dsouzai commented Feb 23, 2022

@jdmpapin could you please review?

@dsouzai
Copy link
Contributor Author

dsouzai commented Mar 1, 2022

@jdmpapin review reminder for this.

@jdmpapin jdmpapin self-assigned this Mar 2, 2022
Copy link
Contributor

@jdmpapin jdmpapin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with a few very minor comments

There's a typo in the commit message of b9f31905 where it says definedGuaranteedID (past/adjective) instead of defineGuaranteedID (imperative)

runtime/compiler/control/JITClientCompilationThread.cpp Outdated Show resolved Hide resolved
doc/compiler/aot/SymbolValidationManager.md Outdated Show resolved Hide resolved
@dsouzai
Copy link
Contributor Author

dsouzai commented Mar 2, 2022

@jdmpapin made the requested changes (including the commit message typo). Good for final review now.

@jdmpapin
Copy link
Contributor

jdmpapin commented Mar 2, 2022

Jenkins test sanity+aot all jdk17

1 similar comment
@dsouzai
Copy link
Contributor Author

dsouzai commented Mar 3, 2022

Jenkins test sanity+aot all jdk17

@jdmpapin
Copy link
Contributor

jdmpapin commented Mar 3, 2022

/home/jenkins/workspace/Build_JDK17_aarch64_linux_Personal/openj9/runtime/compiler/codegen/J9AheadOfTimeCompile.cpp:1290:61: error: 'class TR::SymbolValidationManager' has no member named 'getIDFromSymbol'; did you mean 'getValueFromSymbolID'?
          thunkRecord->setThunkID(reloTarget, symValManager->getIDFromSymbol(svmRecord->_thunk));
                                                             ^~~~~~~~~~~~~~~
                                                             getValueFromSymbolID
/home/jenkins/workspace/Build_JDK17_aarch64_linux_Personal/openj9/runtime/compiler/codegen/J9AheadOfTimeCompile.cpp:1291:62: error: 'class TR::SymbolValidationManager' has no member named 'getIDFromSymbol'; did you mean 'getValueFromSymbolID'?
          thunkRecord->setMethodID(reloTarget, symValManager->getIDFromSymbol(svmRecord->_method));
                                                              ^~~~~~~~~~~~~~~
                                                              getValueFromSymbolID

Looks like you'll need to rebase and update these lines from #14542

@jdmpapin
Copy link
Contributor

jdmpapin commented Mar 3, 2022

Jenkins test sanity+aot all jdk17

@dsouzai
Copy link
Contributor Author

dsouzai commented Mar 4, 2022

jenkins woes again :(

@dsouzai
Copy link
Contributor Author

dsouzai commented Mar 4, 2022

Jenkins test sanity+aot all jdk17

1 similar comment
@jdmpapin
Copy link
Contributor

jdmpapin commented Mar 4, 2022

Jenkins test sanity+aot all jdk17

@AdamBrousseau
Copy link
Contributor

Jenkins test sanity.functional+aot all jdk17

@dsouzai
Copy link
Contributor Author

dsouzai commented Mar 7, 2022

AIX test failed because of

[2022-03-05T02:28:56.690Z]  [ERR] Exception in thread "main" java.awt.AWTError: Can't connect to X11 window server using 'unix:0' as the value of the DISPLAY variable.
[2022-03-05T02:28:56.690Z]  [ERR] 	at java.desktop/sun.awt.X11GraphicsEnvironment.initDisplay(Native Method)
[2022-03-05T02:28:56.690Z]  [ERR] 	at java.desktop/sun.awt.X11GraphicsEnvironment$1.run(X11GraphicsEnvironment.java:101)
...

which, even though it happens on the second run, has been seen before outside of this PR: #14421 (comment)

@jdmpapin This PR should be good for merging now.

@jdmpapin jdmpapin merged commit 03b6564 into eclipse-openj9:master Mar 7, 2022
@dsouzai dsouzai deleted the svmNomenclature branch February 3, 2023 16:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants