search

eclipse-openj9 / openj9

Eclipse OpenJ9: A Java Virtual Machine for OpenJDK that's optimized for small footprint, fast start-up, and high throughput. Builds on Eclipse OMR (https://github.com/eclipse/omr) and combines with the Extensions for OpenJDK for OpenJ9 repo.
Other
3.29k stars 722 forks source link

Investigate and Fix issues with RegDepCopyRemoval Optimization with SplitPostGRA #9712

Open r30shah opened 4 years ago

r30shah commented 4 years ago

For the ValueTypes, certain transformations done in Lowering trees uses SplitPostGRA to split the block with Global Register Dependencies. RegDepCopyRemoval optimization which works in minimizing the register shuffling for GlRegDeps was generating trees which was not expected by SplitPostGRA and it was throwing Fatal Assert. I took a crack at fixing the issue by making changes in both optimization and block splitter in https://github.com/eclipse/omr/pull/5219. Changes works functionally fine but when I ran a DayTrader, I observed regression amounting 5 %.

We made a decision to disable the RegDepCopyRemoval optimization for Value Types. I am opening up this issue to keep track of investigation to fix the regression and enable the optimization.

r30shah commented 4 years ago

FYI @andrewcraik @Leonardo2718

jdmpapin commented 1 month ago

I took a crack at fixing the issue by making changes in both optimization and block splitter in eclipse/omr#5219. Changes works functionally fine but when I ran a DayTrader, I observed regression amounting 5 %.

Later on, (presumably an updated version of) that PR was merged after it was found to no longer cause any such regression: https://github.com/eclipse/omr/pull/5219#issuecomment-684836124

Can eclipse/omr#5250 be reversed now?

r30shah commented 1 month ago

Yes, I think it should be reversed now and we can close https://github.com/eclipse/omr/pull/5219. It just skipped out of radar. The PR trivial enough, though ValueTypes have been matured a lot after that so we need to ensure it passes relevant J9 tests before reversing it. Do you want me to open up PR or you want to open ?

jdmpapin commented 1 month ago

It's probably easier for you to do it - if you don't mind - since you were already working with value types when you did the fix

r30shah commented 1 month ago

Cool, I will open up the PR soon once I finish testing.