Question/issue about GT_COMMA on rhs of GT_ASG.

[sandreenko]

If somehow import generates a tree like this:

[01] *  ASG       struct (copy)
[02] +--*  LCL_VAR   struct<System.Threading.Tasks.ValueTask, 16>(P) V05 tmp3                
[03] \--*    COMMA     struct
[04]    +--* NOP
[05]    \--* LCL_VAR   struct<System.Threading.Tasks.ValueTask, 16> V01 loc0   

then when we come to fgMorph next events happen:

1. LocalAddressVisitor visits [02], but doesn't mark it as address taken or do-not-enregister (because it is not);
2. fgMorphBlkNode tranforms it into:

[01] *  ASG       struct (copy)
[02] +--*  LCL_VAR   struct<System.Threading.Tasks.ValueTask, 16>(P) V05 tmp3  
[06] \--*  OBJ       struct<System.Threading.Tasks.ValueTask, 16>        
[03]    \--*    COMMA     byref
[04]       +--* NOP
[07]       \--* ADDR byref
[05]          \--* LCL_VAR   struct<System.Threading.Tasks.ValueTask, 16> V01 loc0  

creating address node after LocalAddressVisitor is finished;

runtime/src/coreclr/src/jit/morph.cpp

Lines 9442 to 9450 in 0812d49

	// In order to CSE and value number array index expressions and bounds checks,
	// the commas in which they are contained need to match.
	// The pattern is that the COMMA should be the address expression.
	// Therefore, we insert a GT_ADDR just above the node, and wrap it in an obj or ind.
	// TODO-1stClassStructs: Consider whether this can be improved.
	// Example:
	// before: [3] comma struct <- [2] comma struct <- [1] LCL_VAR struct
	// after: [3] comma byref <- [2] comma byref <- [4] addr byref <- [1] LCL_VAR struct

3. Now V01 is address taken, but if it was promoted, then it could be promoted independently (because V01 did not and does not have lvaDoNotEnreg or lvaAddressTaken);
4. ADDR(LCL_VAR V01) is replaced by LCL_VAR_ADDR, ASG(OBJ) is replaced by STORE_OBJ;
5. lvaComputeRefCounts visits LCL_VAR_ADDR and marks only promoted fields as referenced,
   not the variable itself, because the promotion is PROMOTION_TYPE_INDEPENDENT:
   https://github.com/dotnet/runtime/blob/master/src/coreclr/src/jit/compiler.hpp#L1754-L1756
6. genCodeForStoreBlk fails because V01 doesn't have a stack location because we think it is unreferenced:
   ;* V05 tmp3 [V05 ] ( 0, 0 ) struct (16) zero-ref "fgInsertCommaFormTemp is creating a new local variable"
   the actual assert that is firing is assert((varDsc->lvIsParam && !varDsc->lvIsRegArg) || isPrespilledArg); in lvaFrameAddress called from emitIns_R_R_S_S.

The final tree will look like:

N029 (  3,  2) [000034] -c-----N----        t34 =    LCL_VAR_ADDR byref  V05 tmp3         
                                                  *    ref    V05._obj (offs=0x00) -> V07 tmp5          (last use)
                                                  *    short  V05._token (offs=0x08) -> V08 tmp6          (last use)
                                                  *    bool   V05._continueOnCapturedContext (offs=0x0a) -> V09 tmp7          (last use)
                                                  /--*  t34    byref  
N033 ( 54, 37) [000082] -c-XGO------        t82 = *  IND       struct <l:$541, c:$540>
N034 (  3,  2) [000078] Dc-----N----        t78 =    LCL_VAR_ADDR byref  V06 tmp4         d:1
                                                  /--*  t78    byref  
                                                  +--*  t82    struct 
               [000098] nA-XGO------              *  STORE_BLK struct<System.Threading.Tasks.ValueTask, 16> (copy) (Unroll)

So, in short, our IR doesn't accept COMMA as RHS on ASG (as LHS as well, but it is another story) and we have a few workarounds for that, for example:
dotnet/coreclr#23141
or

runtime/src/coreclr/src/jit/optcse.cpp

Lines 2633 to 2638 in 0812d49

	GenTree* asg = m_pCompiler->gtNewTempAssign(cseLclVarNum, val);
	GenTree* origAsg = asg;
	if (!asg->OperIs(GT_ASG))
	{
	// This can only be the case for a struct in which the 'val' was a COMMA, so

If we don't want to fix it, then we could add a few checks that prevent it earlier and more obviously.
Also, the fact that ADDR node can appear after LocalAddressVisitor looks dangerous to me, does anybody have an idea how can it be made safer?

Note: I do not have an example where we create trees like the described in the master branch; that is from one of my struct changes. However, I thought it would be useful to open an issue and make sure that we are aware of such behavior.

category:design
theme:ir
skill-level:intermediate
cost:small

[sandreenko]

PTAL @CarolEidt @mikedn @dotnet/jit-contrib

[CarolEidt]

I think that morph is probably the main source of the issue. If either the source or destination of a block op is a promoted struct, fgMorphCopyBlock() or fgMorphInitBlock() should be morphing the assignment to be one or more assignments of the field lclVars, and if for some reason it can't it should mark the parent as lvDoNotEnregister, forcing them to the stack.

The method fgMorphBlkNode mostly exists to ensure that any COMMAs are in the position expected by CSE. However, it would seem that it should be able to delete the simple case where the "side effect" part of the COMMA is a NOP, such as this case here.

Ideally, it would be good to see these commas eliminated, but IIRC trying to do so resulted in worse code (and more temps). However, if we don't do that, then I think that these methods need to use something stronger than IsLocalAddrExpr() which will return false if it encounters a COMMA - either a new method or extend that to handle them. Maybe fgMorphBlkNode() should do search, and set lvDoNotEnregister if it encounters a LCL_VAR under one of these address expressions that contains a COMMA.

[mikedn]

Also, the fact that ADDR node can appear after LocalAddressVisitor looks dangerous to me, does anybody have an idea how can it be made safer?

Well, the expectancy is that if a local becomes address taken after LocalAddressVisitor then:

• whoever does this also marks the variable address exposed
• there is no need to mark the variable address exposed, typically because the tree is recognized by IsLocalAddrExpr.
  The copy block morph is the later case but it's obvious that it's not going to work as expected because IsLocalAddrExpr (and its myriad clones) do not recognize such COMMAs.

Otherwise it's relatively rare for a variable to become address taken/exposed after LocalAddressVisitor:

• Implicit by ref args call arg morphing creates new local and marks it address exposed
• Return buffer may do this as well but I don't remember where exactly, I think it also happens during call morphing so it needs to manually mark the variable address exposed
• Struct assignment rationalization takes the local address in order to pass it to STORE_OBJ, this is the address taken only case (and it's a tree generated by rationalization so it wouldn't contain a COMMA to begin with).
• Copy block morph can also take the address of a local when promoting, it manually marks the variable address exposed. It really shouldn't do any of this, it should just use LCL_FLD but that's another story.
• Most other cases are also likely related to copy/init block morphing and they're likely artificial, caused by struct related limitations such as no support for struct typed LCL_FLDs.

Removal of ADDR and IsLocalAddrExpr would have probably avoided such a case but since I'm no longer working on that, I'm not sure what's the next best thing that can be done to avoid this.

Luckily COMMAs are only created in limited circumstances. But that "limited" is also likely to cause problems - nobody knows exactly that those circumstances are thus it's difficult to handle all of such circumstances in code (and cover this handling with tests).

So, in short, our IR doesn't accept COMMA as RHS on ASG (as LHS as well, but it is another story) and we have a few workarounds for that, for example:

COMMA as RHS should work (well, most of the time), here the COMMA is under an indir and I think the main problem is that IsLocalAddrExpr & co. do not handle this case. Otherwise it looks like perfectly valid IR to me. After all, COMMA is pretty much a binary op like any other binary op, like ADD for example. The only COMMA use case that I consider to be truly horrific is when it's the LHS of ASG, since there it no longer returns a value like any binary op, instead it "returns" a "location" or whatever you want to call that. Removal of ASG would get rid of this case.

[sandreenko]

Thanks, @CarolEidt and @mikedn, for the detailed analysis.

Carol: I think that morph is probably the main source of the issue. If either the source or destination of a block op is a promoted struct, fgMorphCopyBlock() or fgMorphInitBlock() should be morphing the assignment to be one or more assignments of the field lclVars, and if for some reason it can't it should mark the parent as lvDoNotEnregister, forcing them to the stack.

That will allow is to keep independent promotion and enregister the fields, that is obviously good, but it will require a lot of work.

Mike: COMMA as RHS should work (well, most of the time), here the COMMA is under an indir and I think the main problem is that IsLocalAddrExpr & co. do not handle this case. Otherwise it looks like perfectly valid IR to me. After all, COMMA is pretty much a binary op like any other binary op, like ADD for example.

This approach would be easier and safer, at least as an assert() check if we don't prove that we want to allow IR like this.

Mike: The only COMMA use case that I consider to be truly horrific is when it's the LHS of ASG, since there it no longer returns a value like any binary op, instead it "returns" a "location" or whatever you want to call that. Removal of ASG would get rid of this case.

From what I found it is done in exactly one place without a reason, so that can be fixed easily 17cec7c

That IR doesn't make sense for me as well, it is why we get rid of it right after we create it 49bc2b2

[mikedn]

From what I found it is done in exactly one place without a reason, so that can be fixed easily 17cec7c

Hmm, not familiar with that. The LHS COMMA case I had in mind comes from array INDEX morphing. That too can be avoided but the change is a bit more involved. Oh well, at least that one does not involve local variable access so it's not relevant to this particular issue.

[AndyAyersMS]

@SingleAccretion is there a tracking issue for all the bits and pieces needed to get rid of ADDR/ASG?

[BruceForstall]

@SingleAccretion is there a tracking issue for all the bits and pieces needed to get rid of ADDR/ASG?

#11057