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

Refactor Power l2aEvaluator #2240

Open
0xdaryl opened this issue Jan 18, 2018 · 1 comment
Open

Refactor Power l2aEvaluator #2240

0xdaryl opened this issue Jan 18, 2018 · 1 comment

Comments

@0xdaryl
Copy link
Contributor

0xdaryl commented Jan 18, 2018

OMR::Power::TreeEvaluator::l2aEvaluator contains an undocumented specialty optimization to insert a prefetch when compiled at scorching for a compressed reference sequence. I believe this is an optimization specific to OpenJ9, and hence this function should be refactored to remove it from OMR. The addPrefetch logic (and its dependencies) should also be relocated to OpenJ9.

@ymanton @gita-omr : do you have any insight on the purpose of this prefetch at scorching and whether it actually would be generally useful?

@ymanton
Copy link
Member

ymanton commented Jan 18, 2018

I don't recall seeing this sort of prefetch being emitted lately. The logic in addPrefetch is quite elaborate so I'd be willing to bet something in the IL changed long ago to make this very unlikely to ever be emitted, but that's just my recollection. It should be moved to OpenJ9 if it's still being emitted and has any performance benefits, but if not it should just be removed altogether because its narrow scope and complexity isn't worth maintaining.

IBMJimmyk added a commit to IBMJimmyk/omr that referenced this issue Jun 5, 2018
This removes addPrefetch from OMR. It is replaced by a call to the method
insertPrefetchIfNecessary. By default insertPrefetchIfNecessary just
returns. Extending projects can override it to add functionality.

iloadEvaluator and l2aEvaluator were also modified to now use
insertPrefetchIfNecessary instead of addPrefetch.

Prefetch was added to iloadEvaluator on Power because it was shown to
improve performance on an internal appserver benchmark.

The OpenJ9 changes will be handled by a pull request in OpenJ9.

Issue: eclipse#2240
Issue: eclipse#2634
Signed-off-by: jimmyk <jimmyk@ca.ibm.com>
IBMJimmyk added a commit to IBMJimmyk/omr that referenced this issue Jun 18, 2018
This removes addPrefetch from OMR. It is replaced by a call to the method
insertPrefetchIfNecessary. By default insertPrefetchIfNecessary just
returns. Extending projects can override it to add functionality.

iloadEvaluator and l2aEvaluator were also modified to now use
insertPrefetchIfNecessary instead of addPrefetch.

Prefetch was added to iloadEvaluator on Power because it was shown to
improve performance on an internal appserver benchmark.

The OpenJ9 changes will be handled by a pull request in OpenJ9.

Issue: eclipse#2240
Issue: eclipse#2634
Signed-off-by: jimmyk <jimmyk@ca.ibm.com>
IBMJimmyk added a commit to IBMJimmyk/omr that referenced this issue Jun 18, 2018
This removes addPrefetch from OMR. It is replaced by a call to the method
insertPrefetchIfNecessary. By default insertPrefetchIfNecessary just
returns. Extending projects can override it to add functionality.

iloadEvaluator and l2aEvaluator were also modified to now use
insertPrefetchIfNecessary instead of addPrefetch.

Prefetch was added to iloadEvaluator on Power because it was shown to
improve performance on an internal appserver benchmark.

The OpenJ9 changes will be handled by a pull request in OpenJ9.

Issue: eclipse#2240
Issue: eclipse#2634
Signed-off-by: jimmyk <jimmyk@ca.ibm.com>
0dvictor pushed a commit to 0dvictor/omr that referenced this issue Jun 19, 2018
This removes addPrefetch from OMR. It is replaced by a call to the method
insertPrefetchIfNecessary. By default insertPrefetchIfNecessary just
returns. Extending projects can override it to add functionality.

iloadEvaluator and l2aEvaluator were also modified to now use
insertPrefetchIfNecessary instead of addPrefetch.

Prefetch was added to iloadEvaluator on Power because it was shown to
improve performance on an internal appserver benchmark.

The OpenJ9 changes will be handled by a pull request in OpenJ9.

Issue: eclipse#2240
Issue: eclipse#2634
Signed-off-by: jimmyk <jimmyk@ca.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants