Add metadata to allow client to log vectorization #23065
Conversation
runtime/oti/j9nonbuilder.h
Outdated
| struct J9JITInvokeBasicCallInfo* invokeBasicCallInfo; | ||
| #if defined(J9VM_OPT_JITSERVER) | ||
| // corresponds to J9::Compilation::_vectorApiTransformationPerformed | ||
| bool _vectorApiTransformationPerformed; |
There was a problem hiding this comment.
I wonder whether we could use UDATA flags; for this purpose
There was a problem hiding this comment.
We could also use the type BOOLEAN as used in other places within this file
There was a problem hiding this comment.
No need to add a new field since one is already provided (flags) and it's very appropriate for the job.
| // this list will be copied into the metadata | ||
| if (metaData->_vectorApiTransformationPerformed) | ||
| { | ||
| TR_VerboseLog::writeLineLocked(TR_Vlog_VECTOR_API, "Jit server processed vector ops"); |
There was a problem hiding this comment.
Please correct alignment. Also your code separated the comment from the code it was referring to.
Also, we typically write JITServer (instead of Jit server)
| { | ||
| data->flags |= JIT_METADATA_IS_DESERIALIZED_COMP; | ||
| } | ||
| data->_vectorApiTransformationPerformed = comp->setVectorApiTransformationPerformed(); |
There was a problem hiding this comment.
It's better to define a flag write to data->flags as you see above.
There was a problem hiding this comment.
Also, we need to do this only for outOfProcessCompilation()
mpirvu
left a comment
mpirvu
left a comment
There was a problem hiding this comment.
There are some places in VectorAPIExpansion.hpp that also issue vlog messages. Those need to be covered as well.
| { | ||
| data->flags |= JIT_METADATA_IS_DESERIALIZED_COMP; | ||
| } | ||
| data->_vectorApiTransformationPerformed = comp->setVectorApiTransformationPerformed(); |
There was a problem hiding this comment.
Also, we need to do this only for outOfProcessCompilation()
KavinSatheeskumar
force-pushed
the
vector_api_test_fix
branch
from
9998400 to
c14a580
Compare
| { | ||
| data->flags |= JIT_METADATA_IS_DESERIALIZED_COMP; | ||
| } | ||
| data->flags |= comp->setVectorApiTransformationPerformed() * JIT_METADATA_HAS_BEEN_VECTORIZED; |
There was a problem hiding this comment.
Sorry, but this code makes no sense to me.
There was a problem hiding this comment.
What threw me off is the usage of a setter (based on the name). The name should indicate a getter.
There was a problem hiding this comment.
I would prefer the code to look this:
if (comp->getVectorApiTransformationPerformed())
data->flags |= JIT_METADATA_HAS_BEEN_VECTORIZED;
Also, JIT_METADATA_HAS_BEEN_VECTORIZED is a bit misleading because it can be read that the metadata has been vectorized. Maybe replace it with JIT_METADATA_VECTORIZED_CODE
| */ | ||
| struct J9JITInvokeBasicCallInfo* invokeBasicCallInfo; | ||
| #if defined(J9VM_OPT_JITSERVER) | ||
| // corresponds to J9::Compilation::_vectorApiTransformationPerformed |
There was a problem hiding this comment.
This comment needs to be removed
|
Could you please describe the whole design in the commit message? Is it exactly as described here: #23038 (comment) ? |
3 participants
Fixes #23038
This is the complete design of the fix as described in the issue linked above.