JIT: refactor how opaque VNs are represented #55853
Conversation
Use a unary function to capture opaque VN loop dependence, rather than factoring that out as part of the owning chunk. As a result the VN chunk data no longer has a per-loop component (no dependence on MAX_LOOP_NUM). This gives us the flexibility to address dotnet#54118 by doing something similar for `MapUpdate` VNs.
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
|
@briansull PTAL No diffs (per SPMI). I can add a second commit with the remainder of the fix for #54118 here, but would prefer to merge this as a preparatory no-diff change. |
| @@ -6582,21 +6587,6 @@ bool Compiler::optVNIsLoopInvariant(ValueNum vn, unsigned lnum, VNToBoolMap* loo | |||
| } | |||
| } | |||
| } | |||
| else | |||
There was a problem hiding this comment.
You are removing this block of code.
How was this used prior to your changes?
There was a problem hiding this comment.
The then clause now handles that bit of logic (via the VNF_MemOpaque addition).
| @@ -454,7 +454,7 @@ ValueNumStore::ValueNumStore(Compiler* comp, CompAllocator alloc) | |||
| // We have no current allocation chunks. | |||
| for (unsigned i = 0; i < TYP_COUNT; i++) | |||
| { | |||
| for (unsigned j = CEA_None; j <= CEA_Count + BasicBlock::MAX_LOOP_NUM; j++) | |||
| for (unsigned j = CEA_Const; j <= CEA_Count; j++) | |||
There was a problem hiding this comment.
Is this change a bug-fix, as we weren't fully initializing things?
There was a problem hiding this comment.
I see now that you are just removing the MAX_LOOP_NUM stuff.
Your change deletes CEA_None as well
There was a problem hiding this comment.
Yep, we no longer need the CEA_None chunks.
src/coreclr/jit/valuenum.cpp
Outdated
| Chunk* c = GetAllocChunk(typ, CEA_Func1); | ||
| unsigned offsetWithinChunk = c->AllocVN(); | ||
| ValueNum resultVN = c->m_baseVN + offsetWithinChunk; | ||
| reinterpret_cast<VNDefFunc1Arg*>(c->m_defs)[offsetWithinChunk] = fstruct; |
There was a problem hiding this comment.
This assignment looks odd and is hard to parse.
If it can't be written as a typical assignment, then add a comment with an explanation.
There was a problem hiding this comment.
You mean the last line there? That is what all of the VNForFunc overloads do -- I just "inlined" the unary case here since for these opaque VNs we don't memoize.
There was a problem hiding this comment.
I see it written here using two lines:
ValueNum ValueNumStore::VnForConst(T cnsVal, NumMap* numMap, var_types varType)
T* chunkDefs = reinterpret_cast<T*>(chunk->m_defs);
chunkDefs[offsetWithinChunk] = cnsVal;
There was a problem hiding this comment.
Ok... might as well change this everywhere it appears.
src/coreclr/jit/valuenum.cpp
Outdated
| if (vn == NoVN) | ||
| if (IsVNFunc(vn)) | ||
| { | ||
| return BasicBlock::MAX_LOOP_NUM; | ||
| VNFuncApp funcApp; | ||
| GetVNFunc(vn, &funcApp); |
There was a problem hiding this comment.
I can't add a suggestion since there's a deleted line in the diff, but GetVNFunc returns a bool so you can combine it with the check.
There was a problem hiding this comment.
Thanks. Will update.
ghost
locked as resolved and limited conversation to collaborators
Use a unary function to capture opaque VN loop dependence, rather than factoring
that out as part of the owning chunk. As a result the VN chunk data no longer
has a per-loop component (no dependence on MAX_LOOP_NUM).
This gives us the flexibility to address #54118 by doing something similar for
MapUpdateVNs.