Add and use a MakeSrcRegOptional that validates IsSafeToContainMem was called

[tannergooding]

The general issue was raised here: #77798 (comment) and has surfaced in other areas in the past, such as #72673.

This isn't a comprehensive fix, but did grab the most obvious places. There are still a number of remaining areas where we call SetContained and SetRegOptional directly and where we should migrate them towards MakeSrcContained/RegOptional in the future.

[ghost]

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

---

The general issue was raised here: #77798 (comment) and has surfaced in other areas in the past, such as #72673.

This isn't a comprehensive fix, but did grab the most obvious places. There are still a number of remaining areas where we call SetContained and SetRegOptional directly and where we should migrate them towards MakeSrcContained/RegOptional in the future.

Author:	tannergooding
Assignees:	tannergooding
Labels:	area-CodeGen-coreclr
Milestone:	-

[tannergooding]

CC. @dotnet/jit-contrib

[BruceForstall]

There are a few diffs -- can you explain / give examples?

Also looks like a slight throughput regression. Presumably because we're doing containment validity checks that were omitted before?

[tannergooding]

Also looks like a slight throughput regression.

Right. Since SuperPMI is testing checked, the additional validation done by MakeSrcRegOptional will show up. It won't impact release builds.

There are a few diffs -- can you explain / give examples?

The general issue this is fixing is that it isn't safe to mark a node as Contained or RegOptional if there is a side effect or change in value for a node between its evaluation (in the linear order) and usage. So the diffs are places where we were potentially reordering things before.

For example, in ContainCheckCompare we had the following: https://github.com/dotnet/runtime/pull/77895/files#diff-8fe1c38ca575e327ad3227c06d009135287a438045cecbe3301d9a633a46cf0cL5398-L5412

For this logic, we correctly checked IsSafeToContainMem(cmp, op1) before calling MakeSrcContained. However, we didn't have the same check before SetRegOptional. If we had some IR like:

op1 = *  <node with side effect>
op2 = *  CNS_INT       int    0
tmp = *  <node with side effect>
      /--*  op1
      +--*  op2
res = *  CMP

Then containing op1 would fail since IsSafeToContainMem would say false due to tmp having a side effect. However, it would still be marked regOptional and thus the register allocator would think its safe to spill.

[jakobbotsch]

Since SuperPMI is testing checked, the additional validation done by MakeSrcRegOptional will show up. It won't impact release builds.

SPMI throughput uses release builds.

[tannergooding]

SPMI throughput uses release builds.

Ah, ok. I guess its diffs only that's checked. I thought it was both.

The diff here is 0.08%. The place where we're now doing IsSafeToContainMem in places we weren't before is ContainCheckCompare, ContainCheckBoundsChk, and ContainCheckIntrinsic (where previously it was only done if IsContainableMemoryOp was true, a much cheaper check). We also do it for more ContainCheckHWIntrinsic cases (we used to only due to if childNode wasn't a HWIntrinsic node).

Since its so small and its for correctness, we should likely take it. Notably, we've already gotten some significantly more substantial throughput wins in the past few months so this won't "regress" us compared to the last release. We could potentially restrict some containment/regOptional checks to "optimized code only" as well, which would avoid the more expensive checks.