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

Fix argument info propagation in inlining #4566

Merged
merged 2 commits into from Dec 11, 2019
Merged

Conversation

@liqunl
Copy link
Contributor

liqunl commented Nov 15, 2019

Invariant argument preexistence (IAP) stores argument type info in parm symbols. The problem with this is that most type information is site specific.

This PR will fix this issue by stop putting site specific information on parm symbols. The improved type information will be put on prex arg info which will be used by inliner.

@liqunl liqunl requested review from andrewcraik and vijaysun-omr as code owners Nov 15, 2019
@liqunl liqunl force-pushed the liqunl:argprop-fix branch 7 times, most recently from ec13aa0 to ed826b6 Nov 15, 2019
@liqunl liqunl changed the title WIP: Fix argument info propagation in inlining Fix argument info propagation in inlining Nov 20, 2019
@liqunl

This comment has been minimized.

Copy link
Contributor Author

liqunl commented Nov 20, 2019

@andrewcraik May I ask for your review?

@andrewcraik

This comment has been minimized.

Copy link
Contributor

andrewcraik commented Nov 26, 2019

High level question - for all of the places where the class info is 'currently final' eg the class has not been extended and we rely on that fact during compilation to make decision. Are we sure that any decision we make will be protected by an appropriate non-overridden guard or similar? My concern is that if we use notion to derive field info or other things and the class gets extended do we validate the assumptions still hold at the end of the compile and trip an appropriate guard/abort the compile?

@andrewcraik andrewcraik self-assigned this Nov 26, 2019
Copy link
Contributor

andrewcraik left a comment

I have left a number of comments for consideration. I think there are a few things to address before we can look to merge this. That being said getting arg info propagation working without using the parm symbols is a huge improvement over the current functional problems so thank you for all the work to date on getting a new implementation to avoid these shortfalls.

@liqunl

This comment has been minimized.

Copy link
Contributor Author

liqunl commented Nov 27, 2019

High level question - for all of the places where the class info is 'currently final' eg the class has not been extended and we rely on that fact during compilation to make decision. Are we sure that any decision we make will be protected by an appropriate non-overridden guard or similar? My concern is that if we use notion to derive field info or other things and the class gets extended do we validate the assumptions still hold at the end of the compile and trip an appropriate guard/abort the compile?

Yes, during CHTable commit, we'll fail the compilation if the class has been extended. See openj9 CHTable.cpp

This logic is there for many years, this PR doesn't add it nor extend it for broader use.

@andrewcraik

This comment has been minimized.

Copy link
Contributor

andrewcraik commented Nov 28, 2019

I was wanting to make sure the class is correctly recorded - it appears to be.

@liqunl liqunl force-pushed the liqunl:argprop-fix branch 2 times, most recently from 3d4e3e9 to 4669faf Nov 28, 2019
@liqunl

This comment has been minimized.

Copy link
Contributor Author

liqunl commented Nov 29, 2019

@andrewcraik I made some changes according to your comments, could you review again?

@andrewcraik

This comment has been minimized.

Copy link
Contributor

andrewcraik commented Nov 29, 2019

Thanks very much for all the changes - this is looking pretty good now. My main remaining request will be to add doxygen to the new APIs you are adding - existing APIs are fine to leave undocumented since you haven't changed them.

@liqunl liqunl force-pushed the liqunl:argprop-fix branch from 4669faf to c2fbb46 Dec 2, 2019
@andrewcraik

This comment has been minimized.

Copy link
Contributor

andrewcraik commented Dec 4, 2019

Apart from one last minor detail related to compile time this looks good - @cathyzhyi would appreciate a second opinion and then I think we can get this moving through the sanity build process.

@liqunl liqunl force-pushed the liqunl:argprop-fix branch from c2fbb46 to 20eafb8 Dec 4, 2019
@liqunl

This comment has been minimized.

Copy link
Contributor Author

liqunl commented Dec 4, 2019

Added a trace message in LocalOpts.cpp to print the known object index from prex arg.

@andrewcraik

This comment has been minimized.

Copy link
Contributor

andrewcraik commented Dec 5, 2019

@genie-omr build sanity

@andrewcraik

This comment has been minimized.

Copy link
Contributor

andrewcraik commented Dec 5, 2019

@genie-omr build all

@@ -6932,182 +6939,136 @@ int32_t TR_InvariantArgumentPreexistence::perform()

if (argInfo)
{
if (trace())
if (enableTrace)
traceMsg(comp(), "PREX: Populating parmInfo of inlined method %s from argInfo %p\n", feMethod->signature(trMemory()), argInfo);

TR_PrexArgInfo *argInfo = comp()->getCurrentInlinedCallArgInfo();

This comment has been minimized.

Copy link
@cathyzhyi

cathyzhyi Dec 5, 2019

Contributor

can this line be deleted?

This comment has been minimized.

Copy link
@andrewcraik

andrewcraik Dec 9, 2019

Contributor

@liqunl can you respond to this comment? then I think we can get this merged after testing.

This comment has been minimized.

Copy link
@liqunl

liqunl Dec 9, 2019

Author Contributor

Done

@cathyzhyi

This comment has been minimized.

Copy link
Contributor

cathyzhyi commented Dec 6, 2019

This change itself looks good. Had an offline chat with Liqun about the possibility to refactor the code to make it easier to understand but that's out of the scope of this item.

@liqunl liqunl force-pushed the liqunl:argprop-fix branch from 20eafb8 to bc01a98 Dec 9, 2019
@andrewcraik

This comment has been minimized.

Copy link
Contributor

andrewcraik commented Dec 9, 2019

@genie-omr build all

liqunl added 2 commits Nov 14, 2019
Site specific information that is propagated from caller can not be
stored into parm symbols as the method can be called at different call
sites with different argument info. One exception is the fixed type info
on the outer most method's arguments, because the arguments exist before
entering the method.

Signed-off-by: Liqun Liu <liqunl@ca.ibm.com>
Signed-off-by: Liqun Liu <liqunl@ca.ibm.com>
@andrewcraik andrewcraik merged commit 900b94b into eclipse:master Dec 11, 2019
14 checks passed
14 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/eclipse-omr/pr/aix_ppc-64 Build finished.
Details
continuous-integration/eclipse-omr/pr/linux_390-64 Build finished.
Details
continuous-integration/eclipse-omr/pr/linux_aarch64 Build finished.
Details
continuous-integration/eclipse-omr/pr/linux_arm Build finished.
Details
continuous-integration/eclipse-omr/pr/linux_ppc-64_le_gcc Build finished.
Details
continuous-integration/eclipse-omr/pr/linux_x86 Build finished.
Details
continuous-integration/eclipse-omr/pr/linux_x86-64 Build finished.
Details
continuous-integration/eclipse-omr/pr/linux_x86-64_cmprssptrs Build finished.
Details
continuous-integration/eclipse-omr/pr/osx_x86-64 Build finished.
Details
continuous-integration/eclipse-omr/pr/win_x86-64 Build finished.
Details
continuous-integration/eclipse-omr/pr/zos_390-64 Build finished.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
eclipsefdn/eca The author(s) of the pull request is covered by necessary legal agreements in order to proceed!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.