Conservative fix for struct stores in optRemoveRedundantZeroInits#102580
Conservative fix for struct stores in optRemoveRedundantZeroInits#102580EgorBo merged 8 commits intodotnet:mainfrom
Conversation
ghost
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
|
Diffs a bit conservative with a risk to introduce a gc hole (Thanks @SingleAccretion for spotting this issue!). Technically, in many cases such stores won't be converted to calls but any assumptions we can make here are too fragile. I propose we merge this as is and address dotnet/performance regressions if there will be any. We can consider introducing new flags to guarantee that a store won't be converted into a call etc. cc @SingleAccretion @dotnet/jit-contrib |
SingleAccretion
left a comment
SingleAccretion
left a comment
There was a problem hiding this comment.
A test needs to be added.
…have-gc-safepoints
…have-gc-safepoints
|
/azp list |
This comment was marked as resolved.
This comment was marked as resolved.
|
/azp run runtime-coreclr outerloop, runtime-coreclr jitstress, runtime-coreclr gcstress0x3-gcstress0xc, runtime-coreclr gcstress-extra |
|
Azure Pipelines successfully started running 4 pipeline(s). |
|
/azp run runtime-coreclr outerloop, runtime-coreclr jitstress, runtime-coreclr gcstress0x3-gcstress0xc, runtime-coreclr gcstress-extra |
|
Azure Pipelines successfully started running 4 pipeline(s). |
|
When I added the original not refactored commit to #102415, the GC stress results have improved considerably. There were still a few failures, but a lot less than before. It seems the original issue could be responsible for a noticeable fraction of random failures. |
|
@VSadov @jakobbotsch can I get a green approval then? I hope it will indeed reduce number of random gc hole asserts |
src/coreclr/jit/compiler.hpp
Outdated
| // This is quite a conservative fix as it's hard to prove Lower won't do it at this point. | ||
| if (tree->OperIsLocalStore()) | ||
| { | ||
| return lvaTable[tree->AsLclVarCommon()->GetLclNum()].TypeGet() == TYP_STRUCT; |
There was a problem hiding this comment.
| return lvaTable[tree->AsLclVarCommon()->GetLclNum()].TypeGet() == TYP_STRUCT; | |
| return lvaGetDesc(tree->AsLclVarCommon())->TypeGet() == TYP_STRUCT; |
Feel free to include it in a future change...
There was a problem hiding this comment.
I think this should check the tree type instead of the local's type, i. e. return tree->TypeIs(TYP_STRUCT). One can have struct stores for primitive locals.
There was a problem hiding this comment.
Looks simpler so I'm all for it, but it seems unlikely those would end up lowered to calls.
4 participants
Fixes #102577