Add checks for missing SSA numbers #57274

jakobbotsch · 2021-08-12T09:39:51Z

In several places there were cases where we were checking lvaInSsa for
the lcl num of a local tree node, but then proceeded to use the ssa
number directly after. It is possible for a local to be in SSA without
tree nodes themselves having SSA form built for them, for example if
unreachable code makes it to SSA building. This adds some additional
check for missing SSA numbers.

I'm not personally a big fan of not being able to rely on all blocks
having SSA form built, but these checks would for instance have
prevented the JIT crash in #57061. Also, if someone has ideas on a
different way to do this then please don't hold back. Maybe we should
add a BBF_SSA_BUILT instead, and not process basic blocks without this flag?
Or maybe even noway_assert this.

No SPMI diffs with this change.

PTAL @AndyAyersMS cc @dotnet/jit-contrib

SingleAccretion · 2021-08-12T09:52:50Z

src/coreclr/jit/earlyprop.cpp

+        if (treeRhs->OperIsScalarLocal() && lvaInSsa(treeRhs->AsLclVarCommon()->GetLclNum()) &&
+            treeRhs->AsLclVarCommon()->HasSsaName())


Is it possible for HasSsaName to return true yet lvaInSsa be false? Seems counterintuitive.

I agree, but yes, it's possible. For promoted structs that aren't tracked we sometimes give them the same SSA number as their tracked field if LclVarDsc::CanBeReplacedWithItsField returns true.

That interaction is probably something to consider refactoring in .NET 7.0. If it is intended then I doubt the compiler is handling it correctly in all places.

Agree. IIRC if it is the condition I am thinking about, it is the fix for the regressions that came up after we stopped lying about struct types.

I had to lookup IIRC:

IIRC
ABBREVIATION
IIRC :: If I Recall Correctly.
"IIRC, that was in the seventies"

Looks like #41242 is related to this.

SingleAccretion · 2021-08-12T12:22:51Z

Maybe we should add a BBF_SSA_BUILT instead, and not process basic blocks without this flag?

I am not knowledgeable in this area, what would be the downsides to this option? It seems appealing to just have the core optimizer loops change from using for (BasicBlock* block : Blocks()) to for (BasicBlock* block : BlocksInSsa()).

jakobbotsch · 2021-08-12T12:57:25Z

I am not knowledgeable in this area, what would be the downsides to this option? It seems appealing to just have the core optimizer loops change from using for (BasicBlock* block : Blocks()) to for (BasicBlock* block : BlocksInSsa()).

It seems weird to me that we would ever have basic blocks that come through SSA without having anything built for them, because that means we are keeping around basic blocks that the SSA pass knows are unreachable. So maybe this implies that we should noway_assert that we visit all blocks in the SSA pass.

Of course it would still be possible that we add new blocks or trees after SSA where the checks in this PR would then help. But, I would prefer to have something that verifies that all blocks/IR are in expected format between each pass, but that's probably a bigger change.

JulieLeeMSFT · 2021-08-16T22:14:19Z

@jakobbotsch is this a bug fix and do you want to take this to .NET 6?

briansull

This looks like a great improvement.

briansull · 2021-08-16T21:59:55Z

src/coreclr/jit/optimizer.cpp

@@ -7334,7 +7334,7 @@ bool Compiler::optComputeLoopSideEffectsOfBlock(BasicBlock* blk)
                    if (rhsVN != ValueNumStore::NoVN)
                    {
                        rhsVN = vnStore->VNNormalValue(rhsVN);
-                        if (lvaInSsa(lhsLcl->GetLclNum()))
+                        if (lvaInSsa(lhsLcl->GetLclNum()) && lhsLcl->HasSsaName())


Did you review all of the places where we call GetSsaNum() and were missing the guard HasSsaName() ?

Yes, that's exactly what I did. The most problematic cases were when we were calling GetPerSsaData without this guard, since that will index into bad memory.

briansull · 2021-08-16T22:24:36Z

src/coreclr/jit/assertionprop.cpp

@@ -2963,7 +2963,7 @@ GenTree* Compiler::optCopyAssertionProp(AssertionDsc*        curAssertion,

    // Extract the matching lclNum and ssaNum.
    const unsigned copyLclNum = (op1.lcl.lclNum == lclNum) ? op2.lcl.lclNum : op1.lcl.lclNum;
-    unsigned       copySsaNum = BAD_VAR_NUM;
+    unsigned       copySsaNum = SsaConfig::RESERVED_SSA_NUM;


Is this change here the minimal fix?

The SetLclNum call below resets the SSA number, so while this was clearly bogus, it did not cause any issues (because the order was SetSsaNum -> SetLclNum before). However, we only call this function during local assertion prop and swapping the order below did not have any diffs.

The actual fix is one of the guards added in optComputeLoopSideEffectsOfBlock.

briansull · 2021-08-16T22:32:32Z

src/coreclr/jit/valuenum.cpp

+                                    if (lclDefSsaNum != SsaConfig::RESERVED_SSA_NUM)
+                                    {
+                                        lvaTable[lclNum].GetPerSsaData(lclDefSsaNum)->m_vnPair = newLhsVNPair;
+                                        wasInSsa                                               = true;


This is also a fix here as well.

Right, this is another place where we could potentially call GetPerSsaData with a bad SSA num. But here it was a bit harder to get the control flow the same as before since the tree is not available when we call lvaInSsa above, so I had to introduce the variable.

In several places there were cases where we were checking lvaInSsa for the lcl num of a local tree node, but then proceeded to use the ssa number directly after. It is possible for a local to be in SSA without tree nodes themselves having SSA form built for them, for example if unreachable code makes it to SSA building. This adds some additional check for missing SSA numbers.

jakobbotsch · 2021-08-17T08:57:55Z

@JulieLeeMSFT Overnight I found a couple of x86 tests that are still failing after #57061 that this PR fixes, so I would like to have it for .NET 6.

jakobbotsch · 2021-08-17T15:13:27Z

The test failures seem to be jobs that were killed (?) -- at least, the logs end very abruptly.
Another one seems to be failing elsewhere as well: https://runfo.azurewebsites.net/search/tests/?q=started%3A~7%20definition%3Aruntime%20name%3A%22system.diagnostics.tests.processmoduletests.longmodulefilenamesaresupported%22

Given that the CI was completely green before I added two tests (which are passing), I think that this one can be merged. I'll give it a few more hours to see if anyone else has feedback.

JulieLeeMSFT · 2021-08-17T18:53:35Z

Overnight I found a couple of x86 tests that are still failing after #57061 that this PR fixes, so I would like to have it for .NET 6.

Let's do that. If you can fix within a week, we might be able to backport to RC1. If not, we can do it for RC2.
[Edit] Ah, you already merged. Good that it met RC1 timeline.

dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Aug 12, 2021

SingleAccretion reviewed Aug 12, 2021

View reviewed changes

JulieLeeMSFT assigned jakobbotsch Aug 16, 2021

jakobbotsch requested review from briansull and AndyAyersMS August 16, 2021 18:58

briansull approved these changes Aug 16, 2021

View reviewed changes

jaredpar mentioned this pull request Aug 17, 2021

Feed unreliability affecting CI #55449

Closed

jakobbotsch added 2 commits August 17, 2021 10:44

Add two x86 regression tests

e716960

jakobbotsch force-pushed the harden-missing-ssa-nums branch from 16d7e92 to e716960 Compare August 17, 2021 08:49

Remove superfluous newlines

5ec8906

jaredpar mentioned this pull request Aug 17, 2021

Bundle_Is_Extracted test failing intermittently on Linux #43316

Closed

jakobbotsch merged commit 57839c4 into dotnet:main Aug 17, 2021

jakobbotsch deleted the harden-missing-ssa-nums branch August 17, 2021 17:48

runfoapp bot mentioned this pull request Aug 17, 2021

Test failure in System.Diagnostics.Process.Tests #57452

Closed

jakobbotsch mentioned this pull request Aug 23, 2021

Assertion failed 'ssaNum != SsaConfig::RESERVED_SSA_NUM #57916

Closed

ghost locked as resolved and limited conversation to collaborators Sep 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add checks for missing SSA numbers #57274

Add checks for missing SSA numbers #57274

jakobbotsch commented Aug 12, 2021

SingleAccretion Aug 12, 2021

jakobbotsch Aug 12, 2021

jakobbotsch Aug 12, 2021

SingleAccretion Aug 12, 2021 •

edited

Loading

briansull Aug 12, 2021 •

edited

Loading

jakobbotsch Aug 13, 2021

SingleAccretion commented Aug 12, 2021 •

edited

Loading

jakobbotsch commented Aug 12, 2021

JulieLeeMSFT commented Aug 16, 2021

briansull left a comment

briansull Aug 16, 2021

jakobbotsch Aug 17, 2021

briansull Aug 16, 2021

jakobbotsch Aug 17, 2021

jakobbotsch Aug 17, 2021

briansull Aug 16, 2021

jakobbotsch Aug 17, 2021

jakobbotsch commented Aug 17, 2021

jakobbotsch commented Aug 17, 2021

JulieLeeMSFT commented Aug 17, 2021 •

edited

Loading

		if (treeRhs->OperIsScalarLocal() && lvaInSsa(treeRhs->AsLclVarCommon()->GetLclNum()) &&
		treeRhs->AsLclVarCommon()->HasSsaName())

Add checks for missing SSA numbers #57274

Add checks for missing SSA numbers #57274

Conversation

jakobbotsch commented Aug 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SingleAccretion Aug 12, 2021 • edited Loading

Choose a reason for hiding this comment

briansull Aug 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SingleAccretion commented Aug 12, 2021 • edited Loading

jakobbotsch commented Aug 12, 2021

JulieLeeMSFT commented Aug 16, 2021

briansull left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakobbotsch commented Aug 17, 2021

jakobbotsch commented Aug 17, 2021

JulieLeeMSFT commented Aug 17, 2021 • edited Loading

SingleAccretion Aug 12, 2021 •

edited

Loading

briansull Aug 12, 2021 •

edited

Loading

SingleAccretion commented Aug 12, 2021 •

edited

Loading

JulieLeeMSFT commented Aug 17, 2021 •

edited

Loading