Do we run any of these JIT stress modes with naot? If yes, we may need the helper implemented for naot too.

Also, I am not sure about the durable value of this stress mode and helper. I understand that the helper was useful when implementing the change. Do you think that there is high enough probability that we will passing the heap pointers for return buffers by mistake without noticing it in other ways?

Do we run any of these JIT stress modes with naot? If yes, we may need the helper implemented for naot too.

Can't find any evidence that we run jitstress for NAOT even in outerloop and we definitely have no GCStress for it (#107850)

Also, I am not sure about the durable value of this stress mode and helper. I understand that the helper was useful when implementing the change. Do you think that there is high enough probability that we will passing the heap pointers for return buffers by mistake without noticing it in other ways?

I had it locally for R2R too. It seems my test apps fail badly if I remove theimporter code that makes local copies instead of passing heap pointer, even without any explicit stress mode (and the helper), so I presume I can delete it

I don't think it's ok for this to return true without assigninglclVarTreeOut.
I think it would be better to add a new function that checks for the property we want, e.g.PointsOutsideHeap or similar.GenTreeIndir::IsAddressNotOnHeap could probably be switched to use it as well.

@jakobbotsch ah good idea, addressed

Could usegtPeelOffsets here.

Probably best to do it up before the check forop->OperIs(GT_LCL_ADDR), it might also get some cases on the retbuffer

I've already checked that it doesn't find anything new, but I guess wouldn't hurt

addressed

We still have the problem here that this reorders the LHS of the store with the RHS. I think if the LHS has side effects/ordering effects we need to introduce a local and another comma for it to evaluate it before the call.

@jakobbotsch can you elaborate? we spill the destination to a local (even before my change)

Where is the destination spilled to a local? I think if you callimpStoreStruct with a store likeSTORE_BLK(Foo(), Bar()), then the code here will reorderFoo() so that it happens afterBar().

@jakobbotsch I think I've addressed it in4a48e64 Presumably, GT_RET_EXPR doesn't need special treatment, as we don't spill call their by hands

For inlining we already reorder things because of#112053, so regardless it's probably fine. Once that is fixed we can look into if anything is necessary here to keep the LHS before the call as well.

I don't think we can useimpStoreStruct here since this function is called from outside import viagtNewTempStore. I think it needs to create a comma, or only call this function in some cases (see the checks below forGT_COMMA)

um.. whatimpStoreStruct are your referring here? did you mean impStoreToTemp? Also, seems like this function already appends stuff to statements so it's weird expect it to not?

I guess it's just an implicit contract that when gtNewTempStore calls it - it does not?

Yes I meantimpStoreTemp

seems like this function already appends stuff to statements so it's weird expect it to not?

Where does it do that? I think we only do that for theGT_COMMA case, and it has guards to ensure that only happens during import

We spoke offline and Egor convinced me that actually no reordering is happening here, so we don't need to do any spilling here. Sorry about that.