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

Allow VP to remove/upgrade virtual guards with merged HCR/OSR guards #6762

Merged
merged 1 commit into from
Oct 14, 2022

Conversation

jdmpapin
Copy link
Contributor

Allow folding (or for profiled guards, replacement by noppable guards) to proceed even if the guard in question is merged with an HCR or an OSR guard. Guards folded to always go to the cold side need no special handling. Guards folded to always go to the hot side (i.e. to fall through) have any merged HCR and/or OSR guard split out so that the virtual guard can be removed. When a profiled guard is replaced by a noppable guard, the noppable guard has the same merged HCR or OSR guard as the original profiled guard did, if any.

Currently, this only affects guards that can be folded simply based on the value numbers or constraints of the children (e.g. mustBeEqual()), or guards that use VFT test or method test. The logic that folds nonoverridden guards has canFoldNonOverriddenGuard() check separately for canBeRemoved(). In principle it should be fine to relax that check too, but canFoldNonOverriddenGuard() is also used in constrainCall(), which instead of using removeConditionalBranch() has its own way of removing the guard. And it needs its own way. For example, it has to do TR::TransformUtil::removeTree(...), not vp->_curTree->setNode(NULL). It also might not be able to setUnreachablePath() on the edge. So to extend this improvement to the elimination of nonoverridden guards, some of this new logic will need to be factored out into its own function, and that work is left for the future.

Allow folding (or for profiled guards, replacement by noppable guards)
to proceed even if the guard in question is merged with an HCR or an OSR
guard. Guards folded to always go to the cold side need no special
handling. Guards folded to always go to the hot side (i.e. to fall
through) have any merged HCR and/or OSR guard split out so that the
virtual guard can be removed. When a profiled guard is replaced by a
noppable guard, the noppable guard has the same merged HCR or OSR guard
as the original profiled guard did, if any.

Currently, this only affects guards that can be folded simply based on
the value numbers or constraints of the children (e.g. mustBeEqual()),
or guards that use VFT test or method test. The logic that folds
nonoverridden guards has canFoldNonOverriddenGuard() check separately
for canBeRemoved(). In principle it should be fine to relax that check
too, but canFoldNonOverriddenGuard() is also used in constrainCall(),
which instead of using removeConditionalBranch() has its own way of
removing the guard. And it needs its own way. For example, it has to do
TR::TransformUtil::removeTree(...), not vp->_curTree->setNode(NULL). It
also might not be able to setUnreachablePath() on the edge. So to extend
this improvement to the elimination of nonoverridden guards, some of
this new logic will need to be factored out into its own function, and
that work is left for the future.
@jdmpapin
Copy link
Contributor Author

The Mac failure is #6516:

2022-10-12T15:28:03.8651940Z 31: [----------] 19 tests from PortSockTest
...
2022-10-12T15:28:04.9457800Z 31: /Users/runner/work/1/s/fvtest/porttest/omrsockTest.cpp:1209: Failure
2022-10-12T15:28:04.9460540Z 31:       Expected: rc
2022-10-12T15:28:04.9469620Z 31:       Which is: 1
2022-10-12T15:28:04.9484140Z 31: To be equal to: 2
2022-10-12T15:28:04.9485900Z 31: [  FAILED  ] PortSockTest.poll_functionality_basic (1076 ms)
2022-10-12T15:28:04.9487750Z 31: /Users/runner/work/1/s/fvtest/porttest/omrsockTest.cpp:51: Failure
2022-10-12T15:28:04.9490820Z 31:       Expected: privateOmrPortLibrary->sock_bind(privateOmrPortLibrary, *serverSocket, serverSockAddr)
2022-10-12T15:28:04.9493200Z 31:       Which is: -506
2022-10-12T15:28:04.9495310Z 31: To be equal to: 0
2022-10-12T15:28:04.9497060Z 31: /Users/runner/work/1/s/fvtest/porttest/omrsockTest.cpp:1259: Failure
2022-10-12T15:28:04.9499890Z 31:       Expected: privateOmrPortLibrary->sock_accept(privateOmrPortLibrary, serverSocket, &connectedServerSockAddr, &connectedServerSocket)
2022-10-12T15:28:04.9502460Z 31:       Which is: -20
2022-10-12T15:28:04.9504100Z 31: To be equal to: 0
2022-10-12T15:28:04.9505750Z 31: [  FAILED  ] PortSockTest.poll_functionality_many_sockets (1 ms)
2022-10-12T15:28:04.9508210Z 31: [----------] 19 tests from PortSockTest (1082 ms total)
2022-10-12T15:28:04.9509780Z 31: 
2022-10-12T15:28:04.9511550Z 31: [==========] 245 tests from 21 test cases ran. (122829 ms total)
2022-10-12T15:28:04.9513200Z 31: [  PASSED  ] 243 tests.
2022-10-12T15:28:04.9514740Z 31: [  FAILED  ] 2 tests, listed below:
2022-10-12T15:28:04.9516400Z 31: [  FAILED  ] PortSockTest.poll_functionality_basic
2022-10-12T15:28:04.9518220Z 31: [  FAILED  ] PortSockTest.poll_functionality_many_sockets

@jdmpapin
Copy link
Contributor Author

@vijaysun-omr, would you mind reviewing?

@vijaysun-omr
Copy link
Contributor

Yes I will look at it this afternoon

@vijaysun-omr
Copy link
Contributor

I tried thinking of a case when the class that we use (from the inlined method) as the one to check redefinition for in the HCR guard may be the wrong one, but came to the conclusion that the method must be defined in the class that is mentioned on the call node or a superclass. In either case, it seemed like we would have the class mentioned in the call node be redefined (i.e. it is redefined itself OR a superclass is redefined but that should result in the subclasses getting redefined too). Since this is the core change in logic, I think it is good.

@vijaysun-omr
Copy link
Contributor

Jenkins build all

@jdmpapin
Copy link
Contributor Author

The PPC64LE Linux failure is #6571:

[2022-10-14T00:03:56.123Z] 30: [----------] 14 tests from Special/PPCDirectEncodingTest
[2022-10-14T00:03:56.123Z] 30: free(): invalid next size (normal)
[2022-10-14T00:03:56.123Z] 30/30 Test #30: compunittest ......................Child aborted***Exception:   0.80 sec
...
[2022-10-14T00:03:56.123Z] The following tests FAILED:
[2022-10-14T00:03:56.123Z] 	 30 - compunittest (Child aborted)
[2022-10-14T00:03:56.123Z] Errors while running CTest

@vijaysun-omr
Copy link
Contributor

Failing checks are known/unrelated (as per comments) and review is done. Merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants