More Code Cleanup for EH WriteThru#1180
Conversation
These changes arose from the code reviews for the EH WriteThru work. Some are only tangentially related to EH write-thru, were suggested during that review.
|
@dotnet/jit-contrib PTAL |
src/coreclr/src/jit/lclvars.cpp
Outdated
| // | ||
| void Compiler::lvaSetVarLiveInOutOfHandler(unsigned varNum) | ||
| { | ||
| noway_assert(varNum < lvaCount); |
There was a problem hiding this comment.
Do you really need a "noway" assert here? Otherwise using lvaGetDesc below would provide a normal assert.
There was a problem hiding this comment.
Probably not; not sure why I put that in as it.
| } | ||
|
|
||
| if (size == EA_UNKNOWN) | ||
| bool isValidInReg = ((tree->gtFlags & GTF_SPILLED) == 0); |
There was a problem hiding this comment.
isValidInReg is used only in an assert, it would be good to put all this code in an ifdef DEBUG.
src/coreclr/src/jit/instr.cpp
Outdated
| // reg - the register currently holding the value of the local | ||
| // size - the size attributes for the store | ||
| // | ||
| void CodeGen::inst_TT_RV(instruction ins, GenTree* tree, regNumber reg, emitAttr size) |
There was a problem hiding this comment.
It would be nice to reorder parameters to be more like various emitIns functions - ins, size, tree, reg
There was a problem hiding this comment.
Makes sense - this code was only tangentially related to the EHWriteThru changes, but when reviewing my changes I realized that this code was only ever used for GT_LCL_VAR so it made sense to simplify it.
src/coreclr/src/jit/liveness.cpp
Outdated
| lvaSetVarDoNotEnregister(varNum DEBUGARG(DNER_LiveInOutOfHandler)); | ||
| if (isFinallyVar) | ||
| { | ||
| lvaSetVarLiveInOutOfHandler(varNum); |
There was a problem hiding this comment.
the second call to lvaSetVarLiveInOutOfHandler is unnecessary.
There was a problem hiding this comment.
Thanks - I also restructured this a bit.
src/coreclr/src/jit/liveness.cpp
Outdated
| lvaSetVarLiveInOutOfHandler(varNum); | ||
|
|
||
| /* Don't set lvMustInit unless we have a non-arg, GC pointer */ | ||
| /* Don't set lvMustInit unless we have a non-arg, GC pointer */ |
There was a problem hiding this comment.
could you please change it to the current comment format and delete the newline between the comment and the check.
| return; | ||
| } | ||
|
|
||
| VarSetOps::AssignNoCopy(compiler, exceptVars, VarSetOps::MakeEmpty(compiler)); |
There was a problem hiding this comment.
Why was that line moved from identifyCandidatesExceptionDataflow to its caller identifyCandidates?
The only difference that I see is that we now have it empty when (compiler->compHndBBtabCount <= 0) and we don't visit identifyCandidatesExceptionDataflow, is that the case?
However, in this case I do not see any uses of it, they are all protected with the same condition compiler->compHndBBtabCount > 0.
There was a problem hiding this comment.
In my EHWriteThru changes I have modified some uses to make it both cleaner and a bit more efficient. Also, leaving it uninitialized results in weird failures when one forgets to check.
src/coreclr/src/jit/lsra.cpp
Outdated
|
|
||
| if (block->bbPreds == nullptr) | ||
| { | ||
| // We may have throw blocks with no predecessors, or unreachable blocks. |
There was a problem hiding this comment.
Could you please give me an example of such blocks? Are they arm-specific blocks that we can't delete because we use them to continue after a catch block?
There was a problem hiding this comment.
There seem to be at least a couple of cases. We can eliminate control flow where we are for one reason or other unable to delete the associated blocks. Also, some EH blocks appear to have no predecessor.
There was a problem hiding this comment.
That part still is not clear for me. Does it mean that if findPredBlockForLiveIn returns nullptr then all incoming live-in vars will be living on the stack? I do not see where it happens and why it should happen for blocks that are not hasEHBoundaryIn.
We can eliminate control flow where we are for one reason or other unable to delete the associated blocks.
In such cases, I believe we delete all instructions from these block and zero-initialize live-in and live-out sets, don't we?
Also, some EH blocks appear to have no predecessor.
For them, we should probably return after in if (blockInfo[block->bbNum].hasEHBoundaryIn) return nullptr' block.
There was a problem hiding this comment.
Also, some EH blocks appear to have no predecessor.
AFAIK all handlers do not have predecessors, are there exceptions?
There was a problem hiding this comment.
The comment is not entirely correct. If we don't have a predecessor, then we will add DummyDefs to avoid the default behavior that everything's on the stack (except for the case of the first block or blocks with incoming EH flow). However, this adds unnecessary compile-time cost and may actually result in more mismatches at the boundaries. So we're better off to simply use the variable locations from the previous block.
I have encountered non-empty non-EH blocks with no predecessor that resulted from optimizations that deleted the branch to it, but not all the instructions - I believe it is the case that they are marked in such a way that they can't be deleted.
AFAIK all handlers do not have predecessors, are there exceptions?
A finally block, for example, is considered to have an incoming EH boundary (all vars must be on the stack), but it will have predecessors.
|
This is a zero-diff change, including arm altjit diffs, so I'm going to merge this to unblock myself. |
These changes arose from the code reviews for the EH WriteThru work. Some are only tangentially related to EH write-thru, were suggested during that review.