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

WIP: Simplify containsPrepareForOSR check #106

Closed
wants to merge 1 commit into from

Conversation

ncough
Copy link
Contributor

@ncough ncough commented Sep 22, 2017

OSR data flow analysis must skip OSR blocks. This
includes blocks that contain prepareForOSR calls.
However, the check for these blocks consisted of
a walk across its trees. It is cheaper to check
the OSRBlock flag, as prepareForOSR calls must
only appear in such blocks.

@andrewcraik
Copy link
Contributor

Do we want the option of enabling verification of the flag under debug for sanity?

@ncough ncough force-pushed the containsPrepareForOSR branch 2 times, most recently from f44a21e to 0ac1a74 Compare September 25, 2017 18:18
@ncough
Copy link
Contributor Author

ncough commented Sep 25, 2017

I have added the verification check back in, under the debug flag.

OSR data flow analysis must skip OSR blocks. This
includes blocks that contain prepareForOSR calls.
However, the check for these blocks consisted of
a walk across its trees. It is cheaper to check
the OSRBlock flag, as prepareForOSR calls must
only appear in such blocks.

Signed-off-by: Nicholas Coughlin <cnic@ca.ibm.com>
@andrewcraik
Copy link
Contributor

Jenkins test sanity

@andrewcraik
Copy link
Contributor

@AdamBrousseau something up with 390 again looks to me like the failure isn't related to this code - I'd like to merege if we think things are clean.

@andrewcraik
Copy link
Contributor

Jenkins test sanity

@@ -39,18 +39,26 @@ bool TR_FearPointAnalysis::virtualGuardsKillFear()
return feGetEnv("TR_FPAnalaysisGuardsDoNotKillFear") == NULL;
}

static bool containsPrepareForOSR(TR::Block *block)
bool TR_FearPointAnalysis::containsPrepareForOSR(TR::Block *block)
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this function is called from two analyses would it be better somewhere in the OSR infrastructure?

@ncough ncough changed the title Simplify containsPrepareForOSR check WIP: Simplify containsPrepareForOSR check Oct 2, 2017
@ncough ncough closed this Oct 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants