Bytecode round 12 #219

asotona · 2024-08-30T09:15:33Z

Added test for operand values dominance revealed a bug in the BytecodeLift.
Fixed BytecodeLift revealed regression in the roundtrip stability.
Roundtrip instability was caused by a bug in the LocalsTypeMapper.

All the above is fixed and the roundtrip stability is restored.

Please review.

Thanks,
Adam

Progress

Change must not contain extraneous whitespace

Reviewers

Paul Sandoz (@PaulSandoz - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/babylon.git pull/219/head:pull/219
$ git checkout pull/219

Update a local copy of the PR:
$ git checkout pull/219
$ git pull https://git.openjdk.org/babylon.git pull/219/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 219

View PR using the GUI difftool:
$ git pr show -t 219

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/babylon/pull/219.diff

Webrev

Link to Webrev Comment

…llCorpus

… defaults in BytecodeGenerator

bridgekeeper · 2024-08-30T09:16:12Z

👋 Welcome back asotona! A progress list of the required criteria for merging this PR into code-reflection will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2024-08-30T09:16:17Z

@asotona This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

Bytecode round 12

Reviewed-by: psandoz

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been no new commits pushed to the code-reflection branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the code-reflection branch, type /integrate in a new comment.

mlbridge · 2024-08-30T14:11:54Z

Webrevs

PaulSandoz

It's good to see the value-based isDominatedBymethod being used, probably the first time in anger (not much prior testing on that).

How fastidious are you being about round trip stability? Given the following chain of events, where B == bytecode and M == code mode,

B1 -> M1 -> B2 -> M2 -> B3 -> M3

Do you want M1, M2 and M3 to be the same or just M2 and M3?

PaulSandoz · 2024-08-30T20:11:41Z

test/jdk/java/lang/reflect/code/bytecode/TestSmallCorpus.java

                }
            } catch (Throwable t) {
                error("first lift", t);
            }
        }
    }

+    private void verify(String category, CoreOp.FuncOp func) {
+        OpWriter.CodeItemNamerOption naming = null;
+        for (Body body : func.bodies()) {


It's possible to collapse all the loops by traversing the operations, passing in naming as the argument e.g.,

naming = func.traverse(naming, opVisitor((n, op) -> { for (Value v : op.operands()) { if (!op.result().isDominatedBy(v)) { if (n == null) { n = ... } ... } } return n; }));

Yes, I'll change it. Original idea was that more verifications will hook also on Body and Block level, however Op-level verification seem to be enough.

PaulSandoz · 2024-08-30T20:43:58Z

src/java.base/share/classes/java/lang/reflect/code/bytecode/BytecodeGenerator.java

+            }
+        }
+
+        Set<Block> stopBlocks = new HashSet<>();


If i got this right, we are dealing with an equivalent of an uninitialized variable, of a primitive type, in source that will be assigned in divergent code paths, and the compiler will check it's DA. In the model we currently require the modelling var op be initialized with a default constant value for the primitive type. We don't currently have a concept of an uninitialized value (which might help?), so we have to detect that all loads are dominated by stores, some of which are initializing stores (which detects the uninitialized case or a redundant initialization).

This specific bit of code after the dominated by checks determines if there is a path from n's declaring block to the entry block that does not pass through any of dom's declaring blocks? Why is that is important?

Yes, this is about uninitialized variables (of any type), where there are two or more assignments in divergent paths. Concept of uninitialized variable will definitely help to avoid obsolete default constants declaration. This is a detection of such situation, so the default initialization is skipped and the roundtrip is stable.

This code detect there is no path from a VarLoadOp to the entry block that bypass the VarStoreOps. The default initialization is necessary if there is such path. I've just made it more abstract as a dominance test of a dominant set.

I still don't understand why the second check is needed, traversing the predecessors of n's declaring block. If n is not dominated by any values in set doms then presumably such traversal has to encounter the entry block?

The implementation of Value::isDominatedBy defers to block domination:

public boolean isDominatedBy(Value dom) { if (this == dom) { return true; } if (declaringBlock() != dom.declaringBlock()) { return declaringBlock().isDominatedBy(dom.declaringBlock()); } ...

Individual dominations are handled as you describe. It applies for cases like this:

entry -> var -> var store -> var load \ > var store -> var load

However following case needs a test for dominant set:

entry -> var -> var store -> var load \ / > var store

The var load is not dominated by any individual var store, however it is dominated by them.

The test should fail for example on following case:

entry -> var -> var store -> var load \ / ---------

The reason why the second part is focused on blocks only is because ops cannot form a bypass inside one block.

Thanks! Doh, of course, i get it now. I think what you are doing is the equivalent of DA analysis. Perhaps we can rename the method accordingly? e.g., isDefinitelyAssigned (noting it does not distinguish between assignment with a default value or non-assignment in source because we don't currently make that distinction in the model).

What does the bytecode verifier do when it encounters bytecode in the last case? I expect the verifier will be enhanced to support the new object initialization protocol.

Code with redundant default value assignment is valid, it just causes instability (the code grows with each round).

asotona · 2024-09-02T06:12:44Z

It's good to see the value-based isDominatedBymethod being used, probably the first time in anger (not much prior testing on that).

How fastidious are you being about round trip stability? Given the following chain of events, where B == bytecode and M == code mode,

B1 -> M1 -> B2 -> M2 -> B3 -> M3

Do you want M1, M2 and M3 to be the same or just M2 and M3?

Actually I'm testing only M2 vs M3. The first round is affected by strong stack use in the original code and BytecodeLift reflects that. BytecodeGenerator is not capable of complex predictive stack manipulations, it is limited to the one known value on stack. Second lift is then very different from the first one, however subsequent lifts should be stable.

asotona · 2024-09-05T06:03:51Z

Thank you for the review!
/integrate

openjdk · 2024-09-05T06:04:55Z

Going to push as commit 445a57f.

openjdk · 2024-09-05T06:05:02Z

@asotona Pushed as commit 445a57f.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

asotona added 3 commits August 29, 2024 15:24

Added Op operand values declaration dominance verification to TestSma…

50975b3

…llCorpus

fixed lifting of variables without dominant initial store

17bb538

Roundtrip stabilization - deferred redundant var initializations with…

Loading
Loading status checks…

014ee75

… defaults in BytecodeGenerator

asotona added 2 commits August 30, 2024 11:22

TestSmallCorpus actual numbers

Loading
Loading status checks…

191bb20

restored roundtrip stability - fixed LocalsTypeMapper

Loading
Loading status checks…

59c2cec

asotona marked this pull request as ready for review August 30, 2024 14:08

openjdk bot added ready rfr labels Aug 30, 2024

asotona added 2 commits August 30, 2024 16:12

minor corrections

Loading
Loading status checks…

ae7f0cc

minor corrections

Loading
Loading status checks…

60e3d36

PaulSandoz reviewed Aug 30, 2024

View reviewed changes

asotona added 3 commits September 2, 2024 08:59

applied the suggested changes

Loading
Loading status checks…

58dbaf5

minor adjustments

Loading
Loading status checks…

6a730b0

suggested rename

Loading
Loading status checks…

69866f4

PaulSandoz approved these changes Sep 4, 2024

View reviewed changes

openjdk bot added the integrated label Sep 5, 2024

openjdk bot closed this Sep 5, 2024

openjdk bot removed ready rfr labels Sep 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bytecode round 12 #219

Bytecode round 12 #219

asotona commented Aug 30, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Aug 30, 2024

openjdk bot commented Aug 30, 2024 •

edited

Loading

mlbridge bot commented Aug 30, 2024 •

edited

Loading

PaulSandoz left a comment

PaulSandoz Aug 30, 2024

asotona Sep 2, 2024

PaulSandoz Aug 30, 2024 •

edited

Loading

asotona Sep 2, 2024

asotona Sep 2, 2024 •

edited

Loading

PaulSandoz Sep 3, 2024

asotona Sep 4, 2024

PaulSandoz Sep 4, 2024

asotona Sep 4, 2024

asotona commented Sep 2, 2024 •

edited

Loading

asotona commented Sep 5, 2024

openjdk bot commented Sep 5, 2024

openjdk bot commented Sep 5, 2024

Bytecode round 12 #219

Bytecode round 12 #219

Conversation

asotona commented Aug 30, 2024 • edited by openjdk bot Loading

Progress

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Aug 30, 2024

openjdk bot commented Aug 30, 2024 • edited Loading

mlbridge bot commented Aug 30, 2024 • edited Loading

Webrevs

PaulSandoz left a comment

Choose a reason for hiding this comment

PaulSandoz Aug 30, 2024

Choose a reason for hiding this comment

asotona Sep 2, 2024

Choose a reason for hiding this comment

PaulSandoz Aug 30, 2024 • edited Loading

Choose a reason for hiding this comment

asotona Sep 2, 2024

Choose a reason for hiding this comment

asotona Sep 2, 2024 • edited Loading

Choose a reason for hiding this comment

PaulSandoz Sep 3, 2024

Choose a reason for hiding this comment

asotona Sep 4, 2024

Choose a reason for hiding this comment

PaulSandoz Sep 4, 2024

Choose a reason for hiding this comment

asotona Sep 4, 2024

Choose a reason for hiding this comment

asotona commented Sep 2, 2024 • edited Loading

asotona commented Sep 5, 2024

openjdk bot commented Sep 5, 2024

openjdk bot commented Sep 5, 2024

asotona commented Aug 30, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Aug 30, 2024 •

edited

Loading

mlbridge bot commented Aug 30, 2024 •

edited

Loading

PaulSandoz Aug 30, 2024 •

edited

Loading

asotona Sep 2, 2024 •

edited

Loading

asotona commented Sep 2, 2024 •

edited

Loading