Add classes for building IDT and performing abstract interpretation. #17813

mingweiarthurli · 2023-07-18T10:09:40Z

Issue: #10485
This is the phase 3/3 of the BenefitInliner contribution.

~~Note: This PR can be merged only after the contribution of BenefitInliner in OMR (eclipse/omr#5509) is merged.~~
The OMR part has been successfully merged, this PR is ready to get review now.

Please merge this PR after merging eclipse/omr#7268!

The boolean value insideLoop is intended to be kept as a parameter in some methods in AbsInterpreter.cpp since we would like to make as few as possible changes to the siganature of the public methods. However, this pull request doesn't include the loop handling feature, and it will be introduced by another new pull request after this pull request is merged.
Due to time issue, we merged the changes for handling loop into this PR, and there won't be another additional PR for handling loop.

mingweiarthurli · 2023-11-27T16:32:30Z

@jdmpapin Hey Devin, this is the OpenJ9 part of BenefitInliner phase 3 and the OMR part (eclipse/omr#5509) has already been merged, so I think this PR is also ready to get review. Since you are the person who most familiar with our project, would you mind to assign yourself as the reviewer of this PR? Thank you very much in advance!🙂

mingweiarthurli · 2023-12-04T19:04:03Z

@jdmpapin Hey Devin, I'm sorry for disturbing you, but I'm afraid you has missed my last comment. I just would like to kindly ask you if you mind to assign yourself as the reviewer of this PR? Thank you very much! 🙂

karimhamdanali · 2023-12-12T22:45:40Z

hey @jdmpapin I was wondering if you had a chance to review this PR?

jdmpapin · 2023-12-14T15:29:01Z

I'm reviewing now

mingweiarthurli · 2023-12-15T21:32:18Z

Thank you very much Devin!

runtime/compiler/optimizer/J9Optimizer.cpp

runtime/compiler/optimizer/abstractinterpreter/AbsInterpreter.cpp

runtime/compiler/optimizer/abstractinterpreter/IDTBuilder.hpp

runtime/compiler/optimizer/abstractinterpreter/J9IDTBuilder.hpp

runtime/compiler/optimizer/abstractinterpreter/AbsInterpreter.cpp

runtime/compiler/optimizer/abstractinterpreter/AbsInterpreter.hpp

jdmpapin · 2023-12-21T01:13:35Z

runtime/compiler/optimizer/abstractinterpreter/AbsInterpreter.cpp

+   /*** Case 1: No predecessors. Do nothing ***/
+   if (block->getPredecessors().size() == 0)
+      {
+      return;


How are catch blocks handled?

Sorry that I'm a little bit confused about your question. Are you asking about catch blocks because they don't have predecessors?

They have predecessors but they're exception predecessors (getExceptionPredecessors()) as opposed to "regular" predecessors (getPredecessors()). This code looks like it will treat any exception block as though it's unreachable

Don't worry about handling it at this point, though. Could you please just add a comment? e.g.

// FIXME: handle exception blocks

mingweiarthurli · 2024-02-16T20:47:03Z

Hi @jdmpapin, could you please take a look for this PR? Thank you! I think most comments are resolved.

BTW, we've decided to merge the changes for handling loops to this PR since we would like to let all our existing codes merged ASAP. So there won't be another PR for handling loops, and this PR is already able to handle loops.

Also, I mistakenly removed a few changes in our last OMR PR, which is for handling loops. I opened another OMR PR (eclipse/omr#7268) for it, could you please also take a look for it? Please note that OMR PR is a pre-requisition for this PR, so it should be merged before than this PR.

Thank you very much!

mingweiarthurli · 2024-02-29T18:50:33Z

Hey @jdmpapin, I'm sorry to ping you again. I just would like to kindly remind you that if you have some time to spare, would you mind providing a review for the changes we've recently implemented? Thank you very much for your work and time!

jdmpapin · 2024-03-05T15:32:10Z

I'll review soon

mingweiarthurli · 2024-03-05T20:07:11Z

I'll review soon

Thank you very much!

mingweiarthurli · 2024-04-26T02:14:14Z

Hey @jdmpapin, sorry for disturbing you again. Would you mind to resume this PR review recently? Thank you very much!

karimhamdanali · 2024-05-03T08:32:00Z

@jdmpapin I was wondering if you had a chance to review this PR?

jdmpapin · 2024-05-08T15:25:04Z

Hi Mingwei and Karim, so sorry, I haven't been able to get to this. Unfortunately, I think it will probably still be some time before I get back to it again

jdmpapin

Something is conflicting and GitHub doesn't even say what. Before making any other changes, please first take care of some branch hygiene:

Clean up the commit history, since it contains review fixup commits and also a merge
Rebase and fix conflicts

Sorry, I know this is cumbersome. Thanks in advance

To clean up the commit history, assuming you haven't yet made any changes compared to the latest commit in this PR:

$ git checkout 7c0f636b38de5ff741dcb946f9bf990cbca11da0
$ git checkout 994ae3b615c3c4644b54debe547549c2cf422703 -- .
$ git commit
<edit the commit message>
$ git diff 994ae3b615c3c4644b54debe547549c2cf422703 HEAD
<should show no output>
$ git branch -f jack-inliner-phase-3 HEAD
$ git checkout jack-inliner-phase-3

At this point, your branch is good and you can force push. Please comment on this PR when this step is done. I'd like to verify that the code in the branch hasn't changed before moving on.

Next, rebase onto the latest upstream master, fixing conflicts, and force push again. Please leave another comment to let me know what the conflicting changes were.

jdmpapin · 2024-06-12T20:39:55Z

runtime/compiler/optimizer/abstractinterpreter/AbsInterpreter.cpp

+   {
+   if (regionStructure->isNaturalLoop())
+      {
+      for (RegionIterator ri(regionStructure, _region); ri.getCurrent(); ri.next())


In VP we do the extra initial pass over the loop only for outermost loops, i.e. when insideLoop is false. Nested loops are still processed twice: once during the initial pass over the outermost loop, and then again during the final pass over the outermost loop

This looks like it's trying to do what VP does, but by doing two passes over every loop it will take time exponential in the loop nesting depth

jdmpapin · 2024-06-12T20:44:43Z

runtime/compiler/optimizer/J9EstimateCodeSize.cpp

               break;
               }
            case J9BCgoto:
            case J9BCgotow:
               setupLastTreeTop(currentBlock, bc, i, getBlock(comp(), blocks, calltarget->_calleeMethod, i + bci.relativeBranch(), cfg), calltarget->_calleeMethod, comp());
               cfg.addEdge(currentBlock, getBlock(comp(), blocks, calltarget->_calleeMethod, i + bci.relativeBranch(), cfg));
               addFallThruEdge = false;
+               if (bci.relativeBranch() < 0)
+                  cfg.setHasBackEdges();


lookupswitch and tableswitch should also be checked for negative branch offsets

jdmpapin · 2024-06-12T20:49:21Z

runtime/compiler/optimizer/abstractinterpreter/AbsInterpreter.cpp

+   if (_cfg->hasBackEdges()) // may have loops, do the structural analysis
+      {
+      TR_Structure* structure = TR_RegionAnalysis::getRegions(comp(), _cfg); // generating the structures
+      if (!structure) //Fail to generate structures


This case is falling back to the same logic used for acyclic regions, which I expect to be buggy if there are in fact cycles in the CFG. I think it should fail instead, i.e. return false

jdmpapin · 2024-06-12T20:52:15Z

runtime/compiler/optimizer/abstractinterpreter/AbsInterpreter.cpp

+      }
+   else if (!regionStructure->isAcyclic()) // improper region, do not interpret.
+      {
+      return true;


I don't think it's OK to succeed here. The blocks within the improper region will look like they were unreachable

Please make this case fail instead. Irreducible control flow should be very rare in the bytecode anyway

jdmpapin · 2024-06-12T21:30:10Z

runtime/compiler/optimizer/abstractinterpreter/AbsInterpreter.cpp

+      case J9BCi2d: conversionI2d(TR::Int32); break;
+      case J9BCi2f: conversionI2f(TR::Int32); break;
+      case J9BCi2l: conversionI2l(TR::Int32); break;
+      case J9BCi2s: conversionI2s(TR::Int32); break;


It looks like all of these conversion methods take the source value type as a parameter even though it's determined by the conversion operation

jdmpapin · 2024-06-26T22:33:04Z

runtime/compiler/optimizer/abstractinterpreter/AbsInterpreter.cpp

+         return;
+         }
+
+      /*** Case 3.4: None of the predecessors is interpreted. Block is in dead code area. Do nothing ***/


Now with the loop handling included in this PR, it should be fine as long as abstract interpretation fails when it encounters an improper region and when it doesn't successfully build structure

jdmpapin · 2024-06-26T22:36:00Z

runtime/compiler/optimizer/abstractinterpreter/AbsInterpreter.cpp

+         }
+
+      /*** Case 3.2: some blocks not interpreted ***/
+      if (insideLoop && !allPresecessorsInterpreted && oneInterpretedBlock)


I don't agree with the approach described in that comment, but I'd rather not ask for a substantive change here anymore. Could you please add a comment instead? e.g.

// FIXME: state should be conservative at the start of the first pass through a loop

jdmpapin · 2024-06-26T22:51:14Z

runtime/compiler/optimizer/abstractinterpreter/AbsInterpreter.cpp

+                  uint32_t sigLength;
+                  char *sig = pi->getUnresolvedJavaClassSignature(sigLength);
+                  int32_t elementSize = arrayElementSize(sig);
+                  param = createArrayObject(classBlock, true, 0, INT32_MAX, elementSize);


This passes isNonNull=true, but it should be false

jdmpapin · 2024-06-26T23:02:35Z

runtime/compiler/optimizer/abstractinterpreter/AbsInterpreter.cpp

+            TR_OpaqueClassBlock* classBlock = _callerMethod->getClassFromConstantPool(comp(), cpIndex);
+            if (classBlock)
+               {
+               TR_OpaqueClassBlock* jlClass = comp()->fe()->getClassClassPointer(classBlock);
+               TR::AbsValue* value = createNonNullObject(classBlock);
+               state->push(value);
+               }
+            else
+               {
+               state->push(createNullObject());
+               }


There are a few problems here:

createNonNullObject(classBlock) is once again saying that ldc pushes an instance of classBlock rather than an instance of java/lang/Class. It was corrected in an intermediate commit, but then it was changed back for some reason

jlClass is unused even though it's the correct type for value

An ldc for a constant class should never be null

Please try this instead:

TR_OpaqueClassBlock* jlClass = comp()->getClassClassPointer(); if (jlClass == NULL) state->push(createTopObject()); else state->push(createNonNullObject(jlClass));

jdmpapin · 2024-06-26T23:03:12Z

runtime/compiler/optimizer/abstractinterpreter/AbsInterpreter.cpp

+            break;
+            }
+
+         state->push(createTopObject());


If this is a condy of type long or double, then it should push two entries to the stack. At this point I don't want to ask you to detect that case or even detect condy. Since the possibilities other than string and class are all somewhat MethodHandle-related, I think it would be fine for now for this path to cause interpretByteCode() to fail

…handling loops. This is the phase 3/3 of the BenefitInliner contribution. * AbsInterpreter.hpp, AbsInterpreter.cpp: new class files for performing abstract interpretation. They are also able to handle loops during making the decision for inlining. * AbsStackMachineState.hpp, AbsStackMachineState.cpp: new class files for handling abstract operand stacks and abstract operand arrays. * IDTBuilder.hpp, J9IDTBuilder.hpp, J9IDTBuilder.cpp: new class files for building IDT. * J9EstimateCodeSize.hpp, J9EstimateCodeSize.cpp: add method to generate CFG that will be used by building IDT, and add _hasBackEdges flag if there is loop in the code when generate CFG. Co-authored-by: Mingwei Li <[email protected]> Signed-off-by: Cijie Xia <[email protected]>

jdmpapin · 2024-07-15T17:47:35Z

I've verified that the new revision 643741b is a clean squash and rebase of the preexisting series 994ae3b onto 011aecc. (Thankfully I had already fetched 994ae3b, so I was able to compare.) I don't know why GH said there was a conflict before

mingweiarthurli · 2024-07-15T18:56:12Z

I've verified that the new revision 643741b is a clean squash and rebase of the preexisting series 994ae3b onto 011aecc. (Thankfully I had already fetched 994ae3b, so I was able to compare.) I don't know why GH said there was a conflict before

Hi Devin, sorry for that I didn't expect you've reviewed this PR so quickly, so I haven't mentioned this is just a clean squash yet. I will fix the comments soon and submit them in my next force-pushed commit soon.

mingweiarthurli force-pushed the jack-inliner-phase-3 branch from 6d87a19 to 18ae770 Compare July 18, 2023 10:14

BradleyWood added comp:jit depends:omr Pull request is dependent on a corresponding change in OMR labels Jul 18, 2023

mingweiarthurli force-pushed the jack-inliner-phase-3 branch 2 times, most recently from 24b9872 to 277f273 Compare July 19, 2023 07:04

mingweiarthurli mentioned this pull request Oct 27, 2023

BenefitInliner phase 3/3: Classes for doing nested knapsack algorithm and the inlining. eclipse/omr#5509

Merged

jdmpapin self-requested a review December 4, 2023 20:52

jdmpapin reviewed Dec 21, 2023

View reviewed changes

mingweiarthurli mentioned this pull request Feb 16, 2024

BenefitInliner: Add structural analysis by using given CFG and add _hasBackEdges flag when generating CFG eclipse/omr#7268

Open

jdmpapin reviewed Jun 26, 2024

View reviewed changes

mingweiarthurli force-pushed the jack-inliner-phase-3 branch from 994ae3b to 643741b Compare July 15, 2024 01:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add classes for building IDT and performing abstract interpretation. #17813

Add classes for building IDT and performing abstract interpretation. #17813

mingweiarthurli commented Jul 18, 2023 •

edited

Loading

mingweiarthurli commented Nov 27, 2023

mingweiarthurli commented Dec 4, 2023

karimhamdanali commented Dec 12, 2023

jdmpapin commented Dec 14, 2023

mingweiarthurli commented Dec 15, 2023

jdmpapin Dec 21, 2023

mingweiarthurli Feb 16, 2024

jdmpapin Jun 26, 2024

mingweiarthurli commented Feb 16, 2024 •

edited

Loading

mingweiarthurli commented Feb 29, 2024 •

edited

Loading

jdmpapin commented Mar 5, 2024

mingweiarthurli commented Mar 5, 2024

mingweiarthurli commented Apr 26, 2024

karimhamdanali commented May 3, 2024

jdmpapin commented May 8, 2024

jdmpapin left a comment •

edited

Loading

jdmpapin Jun 12, 2024

jdmpapin Jun 12, 2024

jdmpapin Jun 12, 2024

jdmpapin Jun 12, 2024

jdmpapin Jun 12, 2024

jdmpapin Jun 26, 2024

jdmpapin Jun 26, 2024

jdmpapin Jun 26, 2024

jdmpapin Jun 26, 2024

jdmpapin Jun 26, 2024

jdmpapin commented Jul 15, 2024

mingweiarthurli commented Jul 15, 2024 •

edited

Loading

Add classes for building IDT and performing abstract interpretation. #17813

Are you sure you want to change the base?

Add classes for building IDT and performing abstract interpretation. #17813

Conversation

mingweiarthurli commented Jul 18, 2023 • edited Loading

mingweiarthurli commented Nov 27, 2023

mingweiarthurli commented Dec 4, 2023

karimhamdanali commented Dec 12, 2023

jdmpapin commented Dec 14, 2023

mingweiarthurli commented Dec 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mingweiarthurli commented Feb 16, 2024 • edited Loading

mingweiarthurli commented Feb 29, 2024 • edited Loading

jdmpapin commented Mar 5, 2024

mingweiarthurli commented Mar 5, 2024

mingweiarthurli commented Apr 26, 2024

karimhamdanali commented May 3, 2024

jdmpapin commented May 8, 2024

jdmpapin left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdmpapin commented Jul 15, 2024

mingweiarthurli commented Jul 15, 2024 • edited Loading

mingweiarthurli commented Jul 18, 2023 •

edited

Loading

mingweiarthurli commented Feb 16, 2024 •

edited

Loading

mingweiarthurli commented Feb 29, 2024 •

edited

Loading

jdmpapin left a comment •

edited

Loading

mingweiarthurli commented Jul 15, 2024 •

edited

Loading