Skip to content
This repository has been archived by the owner on Aug 22, 2019. It is now read-only.

Simplify ordered float comparisons and add test cases #235

Merged

Conversation

mrigger
Copy link
Collaborator

@mrigger mrigger commented May 3, 2016

No description provided.

return true;
} else {
return false;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know if it's worth profiling at this point? Or is this compare node only ever used in cases which will profile anyway, like conditional jumps?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If the comparison is used in an branch, then the branch probability profiling that is already injected in the successor selection should suffice. However, the value could also be stored into the frame or somewhere else, and then used at a different location. Do you think that branch probability injection could improve performance in this case?

/cc @lukasstadler @gilles-duboscq

Copy link
Contributor

Choose a reason for hiding this comment

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

In Ruby in general if we have an if statement that we don't expect to always be a constant in code that is being PEd, we will use a profile.

Copy link
Member

Choose a reason for hiding this comment

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

Isn't this somehow a larger design question?
Do you profile your conditions/logic nodes or do you profile the things that use them (such as branching control-flow nodes)

Copy link
Member

Choose a reason for hiding this comment

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

IMHO, the latter (branching control flow). Profiling the input condition of an if is a convenient way to do that but the profile still belongs to the if (and its node), not the condition.

@chrisseaton chrisseaton merged commit a62ab87 into graalvm:master May 3, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants