Fold VarHandle held in static final fields with fear #2885

liqunl · 2018-09-17T03:39:07Z

Fold static final fields that hold VarHandle object with OSR guard. The
folding occurs in VP and when OSR infrastructure is still available.

Signed-off-by: Liqun Liu liqunl@ca.ibm.com

liqunl · 2019-01-02T22:29:33Z

@andrewcraik Can I have a review? Will write the PR description later.

andrewcraik · 2019-01-03T21:13:59Z

runtime/compiler/optimizer/J9TransformUtil.cpp

+   {
+   TR_ASSERT(start->getEnclosingBlock() == end->getEnclosingBlock(), "Does not support range across blocks");
+
+   traceMsg(comp, "prohibit final field folding over n%dn - n%dn \n", start->getNode()->getGlobalIndex(), end->getNode()->getGlobalIndex());


this should be dumpOptDetails

andrewcraik · 2019-01-03T21:15:13Z

runtime/compiler/optimizer/J9TransformUtil.cpp

+
+   if (comp->getOption(TR_EnableOSR) &&
+       comp->isOSRTransitionTarget(TR::postExecutionOSR) &&
+       comp->getOSRMode() == TR::voluntaryOSR)


prohibition only works in certain modes - we should probably FATAL_ASSERT if it isn't supported or have some kind of return code. We shouldn't just silently fail.

Added an assertion and StringPeepHoles now checks the mode before prohibition.

andrewcraik · 2019-01-03T21:15:24Z

runtime/compiler/optimizer/J9TransformUtil.cpp

+         {
+         prohibitGuardedStaticFinalFieldFoldingOnNodeAndChildren(comp, tt->getNode(), &visited);
+         tt = tt->getNextTreeTop();
+         } while (tt != ttAfterEnd);


code style - while on next line.

andrewcraik · 2019-01-03T21:15:49Z

runtime/compiler/optimizer/J9TransformUtil.cpp

+       node->getOpCode().isLoadVarDirect() &&
+       node->isLoadOfStaticFinalField())
+      {
+      traceMsg(comp, "prohibit folding on n%dn\n", node->getGlobalIndex());


There should be a some kind of trace flag or something so this doesn't fill the log up.

This is where we set the flag, so I updated the code to use dumpOptDetails here.

andrewcraik · 2019-01-03T21:17:06Z

runtime/compiler/optimizer/J9ValuePropagation.cpp

+static bool skipFinalFieldFoldingInBlock(TR::Block* block)
+   {
+   if (block->isCold() ||
+       TR_FearPointAnalysis::shouldSkipBlock(block) ||


I'm not a fan of this coupling - it isn't obvious why FearPointAnalysis should be connected to VP. The common concept probably belongs in its own header or something with a more sensible name.

Renamed the function to isOSRRelatedBlock and move it to OSRUtils.hpp.

andrewcraik · 2019-01-03T21:18:17Z

runtime/compiler/optimizer/J9ValuePropagation.cpp

@@ -1175,3 +1176,163 @@ J9::ValuePropagation::getParmValues()

   TR_ASSERT(parmIterator->atEnd() && parmIndex == numParms, "Bad signature for owning method");
   }
+
+static bool isTakenSideOfAVirtualGuard(TR::Block* block)


This seems misleading - what about an OSR guard or a virtual guard which is implemented using OSR... Why not just check if the block's predecessors to see if the end in a branch that is a virtual guard?

Changed to check the last tree of the block's predecessor.

andrewcraik · 2019-01-03T21:18:50Z

runtime/compiler/optimizer/J9ValuePropagation.cpp

@@ -43,6 +43,7 @@
 #include "env/VMAccessCriticalSection.hpp"      // for VMAccessCriticalSection
 #include "runtime/RuntimeAssumptions.hpp"
 #include "env/J9JitMemory.hpp"
+#include "optimizer/FearPointAnalysis.hpp"


really not a fan of this - see below we need a more sensible header to hold the common query if there isn't already a suitable OSR header.

andrewcraik · 2019-01-03T21:19:33Z

runtime/compiler/compile/J9SymbolReferenceTable.cpp

@@ -1400,7 +1400,7 @@ J9::SymbolReferenceTable::findOrCreateStaticSymbol(TR::ResolvedMethodSymbol * ow
         TR_OpaqueClassBlock *declaringClass = owningMethod->getDeclaringClassFromFieldOrStatic(comp(), cpIndex);
         if (declaringClass && fej9->isClassInitialized(declaringClass))
            {
-            static const char *dontFoldVarHandle = feGetEnv("TR_DontFoldVarHandle");
+            static const char *foldVarHandleWithoutGuard = feGetEnv("TR_FoldVarHandleWithoutGuard");


I would prefer we delivered the code and then flipped the sense of this so we only deal with one thing at a time to make narrowing things down easier.

Change in this file will be in a different PR.

andrewcraik · 2019-01-03T21:21:31Z

runtime/compiler/optimizer/J9ValuePropagation.cpp

+   if (fieldNode->getByteCodeInfo().doNotProfile() ||
+       skipFinalFieldFoldingInBlock(tree->getEnclosingBlock()) ||
+       !safeToAddFearPointIn(comp(), fieldNode->getByteCodeInfo().getCallerIndex()) ||
+       TR::TransformUtil::canFoldStaticFinalField(comp(), fieldNode) != TR_maybe)


I think this would read better with the ||s a the start of the lines to make the condition relationships more obvious since this is quite complicated.

andrewcraik · 2019-01-03T21:23:11Z

runtime/compiler/optimizer/J9ValuePropagation.cpp

+
+   if (isStaticFinalFieldWorthFolding(comp(), declaringClass, fieldSignature, fieldSigLength))
+      {
+      if (TR::TransformUtil::foldStaticFinalFieldAssumingProtection(comp(), fieldNode))


A perform transformation would seem to be a good idea here...

foldStaticFinalFieldAssumingProtection does a perform transformation.

andrewcraik · 2019-01-03T21:27:03Z

runtime/compiler/optimizer/OSRGuardInsertion.cpp

+          ttNode->getFirstChild()->isOSRFearPointHelperCall())
+         {
+         if (trace())
+            traceMsg(comp(), "Remove osrFearPointHelper call n%dn %p\n", ttNode->getGlobalIndex(), ttNode);


probably should be a dumpOptDetails

andrewcraik · 2019-01-04T20:14:01Z

runtime/compiler/optimizer/J9TransformUtil.cpp

@@ -1916,30 +1916,29 @@ J9::TransformUtil::createDiamondForCall(TR::Optimization* opt, TR::TreeTop *call
 void J9::TransformUtil::removePotentialOSRPointHelperCalls(TR::Compilation* comp, TR::TreeTop* start, TR::TreeTop* end)
   {
   TR_ASSERT(start->getEnclosingBlock() == end->getEnclosingBlock(), "Does not support range across blocks");
+   TR_ASSERT(comp->supportsInduceOSR() && comp->isOSRTransitionTarget(TR::postExecutionOSR) && comp->getOSRMode() == TR::voluntaryOSR,


do we also need to check that the OSR infrastructure hasn't been removed? it doesn't make sense to do this after that point I don't think since we have made a lot of complex decisions.

comp->supportsInduceOSR() does the check.

andrewcraik · 2019-01-04T20:16:44Z

runtime/compiler/optimizer/J9TransformUtil.cpp

+void J9::TransformUtil::prohibitGuardedStaticFinalFieldFoldingOverRange(TR::Compilation* comp, TR::TreeTop* start, TR::TreeTop* end)
+   {
+   TR_ASSERT(start->getEnclosingBlock() == end->getEnclosingBlock(), "Does not support range across blocks");
+   TR_ASSERT(comp->supportsInduceOSR() && comp->isOSRTransitionTarget(TR::postExecutionOSR) && comp->getOSRMode() == TR::voluntaryOSR,


isn't it more correct to say we only do static final field folding in specific modes? the prohibition should work in all modes where we fold?

andrewcraik · 2019-01-04T20:18:47Z

runtime/compiler/optimizer/J9ValuePropagation.cpp

+      return false;
+      }
+
+   if (fieldNode->getByteCodeInfo().doNotProfile()


hmm - I'm not sure I like this use of doNotProfile - whether a static final field read is foldable is not related to whether the field read node came from the original program representation so this would seem an overload of the meaning of the flag likely to confuses other opts

andrewcraik · 2019-01-04T20:19:51Z

runtime/compiler/optimizer/StringPeepholes.cpp

+
+      TR::TransformUtil::removePotentialOSRPointHelperCalls(comp(), startTree, endTree);
+      TR::TransformUtil::prohibitOSROverRange(comp(), startTree, endTree);
+      TR::TransformUtil::prohibitGuardedStaticFinalFieldFoldingOverRange(comp(), startTree, endTree);


I don't think this is necessary - we can fold finals if we can protect the OSR points

andrewcraik · 2019-01-07T20:38:45Z

runtime/compiler/optimizer/OSRUtils.cpp

+
+bool isOSRRelatedBlock(TR::Block *block)
+   {
+   return block->isOSRCatchBlock() || block->isOSRCodeBlock() || containsPrepareForOSR(block);


what blocks contain a prepare that is not a catch or code block?

I was just copying shouldSkipBlock in FearAnalysis. Didn't know that prepareForOSR is in OSR code block. I thought OSR code block is the one with the induce.

andrewcraik · 2019-01-07T20:39:48Z

runtime/compiler/compile/J9Compilation.hpp

@@ -313,6 +313,10 @@ class OMR_EXTENSIBLE Compilation : public OMR::CompilationConnector

   TR::SymbolValidationManager *getSymbolValidationManager() { return _symbolValidationManager; }

+   // Flag to record if any optimization has prohibited OSR over a range of trees
+   void setOSRProhibitedOverRangeOfTrees() { _osrProhibitedOverRangeOfTrees = true; }
+   bool isOSRProhibitedOverRangeOfTrees() { return _osrProhibitedOverRangeOfTrees; }


perhaps hasOSRProhibitions would be a better, shorter name?

hasOSRProhibitions is used on OMR method symbol to record OSR prohibition on bytecodes. I used a different name to not to cause confusion.

andrewcraik · 2019-01-07T20:42:08Z

runtime/compiler/optimizer/J9ValuePropagation.cpp

+   // Due to VirtualGuardHeadMerger, the taken side of a virtual guard may have more than one predecessors,
+   // each containing a virtual guard that branches to the taken side. It's sufficient to look at only
+   // one predecessor.
+   TR::Node* predLastRealNode = block->getPredecessors().size() > 0 ?


VGHM or other opts may also chain the cold side of guards together - see VirtualGuardTailSplitter - it is not sufficient to look at only one.

liqunl · 2019-01-08T21:16:01Z

@andrewcraik Can you review again?

andrewcraik · 2019-01-09T14:56:35Z

runtime/compiler/optimizer/J9TransformUtil.cpp

@@ -1961,24 +1960,24 @@ void J9::TransformUtil::removePotentialOSRPointHelperCalls(TR::Compilation* comp
 void J9::TransformUtil::prohibitOSROverRange(TR::Compilation* comp, TR::TreeTop* start, TR::TreeTop* end)
   {
   TR_ASSERT(start->getEnclosingBlock() == end->getEnclosingBlock(), "Does not support range across blocks");
+   TR_ASSERT(comp->supportsInduceOSR() && comp->isOSRTransitionTarget(TR::postExecutionOSR) && comp->getOSRMode() == TR::voluntaryOSR,


Do we need something to check about if the OSR infrastructure has been removed already since this operation doesn't make sense in that world right?

comp->supportsInduceOSR() does the check.

andrewcraik · 2019-01-09T14:58:12Z

runtime/compiler/optimizer/J9ValuePropagation.cpp

+   }
+
+
+static TR_HCRGuardAnalysis* runHCRGuardAnalysisIfNecessary()


This might be better named runHCRGuardAnalysisIfPossible since we would like it, but we don't have to do it which I think necessary suggests.

andrewcraik · 2019-01-09T15:02:46Z

Ok so I think this new revision where VP is checking the safety of folding with an optional, but not currently supported hook for running HCRGuard analysis seems like a good design now where we don't rely on node flags and we will be conservatively correct. Given the complexity I'd appreciate a second opinion from @jdmpapin but I'm going to start sanity. If there are additional checks added to asserts please do it as a separate commit that can be squashed so we can avoid having to do a full sanity by inspecting the change prior to the squash.

andrewcraik · 2019-01-09T15:03:47Z

Jenkins test sanity xlinux,win,plinux jdk8,jdk11

jdmpapin · 2019-01-10T00:09:52Z

runtime/compiler/optimizer/J9ValuePropagation.cpp

+
+   if (!comp()->supportsInduceOSR()
+       || !comp()->isOSRTransitionTarget(TR::postExecutionOSR)
+       || !comp()->getOSRMode() == TR::voluntaryOSR)


Do you mean comp()->getOSRMode() != TR::voluntaryOSR? I think this will actually work as-is at the moment, but only because of the (implicit) numeric values assigned in OSRMode.

enum OSRMode { voluntaryOSR, // 0 involuntaryOSR // 1 };

So

(!comp()->getOSRMode()) == TR::voluntaryOSR iff (!comp()->getOSRMode()) == 0 iff comp()->getOSRMode() != 0 iff comp()->getOSRMode() != TR::voluntaryOSR

jdmpapin · 2019-01-10T01:06:02Z

runtime/compiler/optimizer/J9ValuePropagation.cpp

+      tt = tt->getPrevTreeTop();
+      }
+
+   TR_HCRGuardAnalysis* guardAnalysis = runHCRGuardAnalysisIfNecessary();


I don't think I see the point of this "hook" - there's no abstraction boundary here. That said, it doesn't actively harm anything. The code checking the results is just dead for now

We haven't done any compile time measured so don't know when we can afford the analysis now. It'll be updated once we done the performance measurement.

jdmpapin · 2019-01-10T01:11:50Z

runtime/compiler/optimizer/J9ValuePropagation.cpp

+      }
+
+   TR_HCRGuardAnalysis* guardAnalysis = runHCRGuardAnalysisIfNecessary();
+   if (guardAnalysis &&  guardAnalysis->_blockAnalysisInfo[block->getNumber()]->isEmpty())


It seems to me that we could get a definite answer (either TR_yes or TR_no) whenever there's a guardAnalysis. Is there some reason this produces TR_maybe instead of TR_no? Although at the moment I suppose the result is only compared with TR_yes so it won't make a difference.

jdmpapin · 2019-01-10T01:40:05Z

runtime/compiler/optimizer/J9ValuePropagation.cpp

+         {
+         traceMsg(comp(), "Not safe to add fear point because caller frame %d cannot OSR\n", callerIndex);
+         }
+      return TR_no;


If we were to insert a fear point here, the points at which OSR would be required to protect it wouldn't necessarily be in the same inlined site. Does this early return just allow for the following early return when !isOSRProhibitedOverRangeOfTrees(), or is there some other reason it's needed?

No. That's something I didn't think of when changing the code. It was needed in the old version when we never want to run HCR guard analysis. Thanks for pointing this out.

Come to think of it, the !isOSRProhibitedOverRangeOfTrees() seems to be broken even though we've checked for cannotAttemptOSRDuring(). Suppose we can attempt OSR at the given tree, but the fear flows out from the beginning of the inlined body and into a different inlined body where we can't, and furthermore suppose that we did no transformations in string peepholes (or similar). Even without running a full "HCR guard analysis" (now a bit of a misnomer), the local analysis below would conservatively prevent folding, if it were allowed to run. But instead we'll return early with TR_yes

edit: The local analysis would only be sure to prevent folding in this case so long as we haven't combined blocks from different methods, which we might do in tree simplification during methodHandleInvokeInliningGroup. Though for most inlined methods their blocks will be isolated within the diamond for their guards, and most methods have at least an HCR guard that can't be eliminated until OSR guard insertion runs.

potentialOSRPointHelper will prevent a fear flowing to an inlined body that cannot attempt OSR.

Ah, right! Thanks, that makes sense 👍

jdmpapin · 2019-01-10T01:57:05Z

runtime/compiler/optimizer/OSRGuardInsertion.cpp

-            //
-            cleanUpPotentialOSRPointHelperCalls();
-            }
-
         if (trace())
            {
            comp()->dumpMethodTrees("Trees after redundant potentialOSRPointHelper call removal", comp()->getMethodSymbol());


Should this tracing move along with the logic to remove potential OSR point helper calls?

jdmpapin · 2019-01-10T02:21:49Z

I suppose VP catches these things before inlining as part of methodHandleInvokeInliningGroup?

liqunl · 2019-01-10T19:15:03Z

I suppose VP catches these things before inlining as part of methodHandleInvokeInliningGroup?

Yes.

liqunl · 2019-01-10T19:16:18Z

@jdmpapin Made some changes in a separate commit, will squash it before merge. Please review again.

Recording the OSR prohibition can help determine if an OSR related optimization is safe without running HCRGuardAnalysis. Signed-off-by: Liqun Liu <liqunl@ca.ibm.com>

Fold static final fields that hold VarHandle object with OSR guard. The folding occurs in VP and when OSR infrastructure is still available. Signed-off-by: Liqun Liu <liqunl@ca.ibm.com>

andrewcraik · 2019-01-10T21:19:33Z

Jenkins test sanity xlinux,win,plinux jdk8,jdk11

andrewcraik

LGTM

liqunl force-pushed the folding branch 2 times, most recently from 6e15125 to e5941b3 Compare September 17, 2018 16:03

pshipton added the comp:jit label Oct 15, 2018

liqunl mentioned this pull request Nov 29, 2018

Guarded static final field folding #3018

Open

7 tasks

liqunl force-pushed the folding branch from e5941b3 to 8f7ae1b Compare December 7, 2018 04:38

liqunl changed the title ~~WIP: Fold static final fields with OSR guards~~ WIP: Fold static final fields with fear Dec 7, 2018

liqunl force-pushed the folding branch from 8f7ae1b to b158c3a Compare January 2, 2019 22:27

andrewcraik reviewed Jan 3, 2019

View reviewed changes

liqunl force-pushed the folding branch 5 times, most recently from e04d659 to d3b1c91 Compare January 4, 2019 20:06

andrewcraik reviewed Jan 4, 2019

View reviewed changes

liqunl force-pushed the folding branch 2 times, most recently from fd32f17 to 40085bf Compare January 7, 2019 16:18

liqunl force-pushed the folding branch from 40085bf to 03d00c4 Compare January 7, 2019 19:55

andrewcraik reviewed Jan 7, 2019

View reviewed changes

liqunl force-pushed the folding branch from 03d00c4 to 01554a9 Compare January 7, 2019 22:05

andrewcraik reviewed Jan 9, 2019

View reviewed changes

liqunl changed the title ~~WIP: Fold static final fields with fear~~ WIP: Fold VarHandle held in static final fields with fear Jan 9, 2019

liqunl changed the title ~~WIP: Fold VarHandle held in static final fields with fear~~ Fold VarHandle held in static final fields with fear Jan 9, 2019

jdmpapin reviewed Jan 10, 2019

View reviewed changes

jdmpapin approved these changes Jan 10, 2019

View reviewed changes

Liqun Liu added 2 commits January 10, 2019 16:12

Record OSR prohibition in Compilation object

369917d

Recording the OSR prohibition can help determine if an OSR related optimization is safe without running HCRGuardAnalysis. Signed-off-by: Liqun Liu <liqunl@ca.ibm.com>

Fold VarHandle held in static final fields

94cc386

Fold static final fields that hold VarHandle object with OSR guard. The folding occurs in VP and when OSR infrastructure is still available. Signed-off-by: Liqun Liu <liqunl@ca.ibm.com>

liqunl force-pushed the folding branch from f93f8d8 to 94cc386 Compare January 10, 2019 21:12

andrewcraik approved these changes Jan 11, 2019

View reviewed changes

andrewcraik merged commit ffbffce into eclipse-openj9:master Jan 11, 2019

		}


		static TR_HCRGuardAnalysis* runHCRGuardAnalysisIfNecessary()

Fold VarHandle held in static final fields with fear #2885

Fold VarHandle held in static final fields with fear #2885

Conversation

liqunl commented Sep 17, 2018 • edited Loading

liqunl commented Jan 2, 2019

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

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

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

liqunl commented Jan 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewcraik commented Jan 9, 2019

andrewcraik commented Jan 9, 2019

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 Jan 10, 2019 • 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

jdmpapin commented Jan 10, 2019

liqunl commented Jan 10, 2019

liqunl commented Jan 10, 2019

andrewcraik commented Jan 10, 2019

andrewcraik left a comment

Choose a reason for hiding this comment

liqunl commented Sep 17, 2018 •

edited

Loading

jdmpapin Jan 10, 2019 •

edited

Loading