[Question] impCloneExpr vs fgInsertCommaFormTemp + gtCloneExpr

[tannergooding]

Looking at various parts of the importer, it looks like one of a couple approaches are used when a node needs to be copied. All of these code paths are functionally similar but with minor tweaks in the actual checks done and when a given node is copied to a local vs cloned. I was wondering if there is a "correct" one that should be getting used and if the various paths should be normalized to a single helper method?

The first is just calling impCloneExpr which calls gtClone if the tree isn't GTF_GLOB_EFFECT and is simple and which creates a new LCL_VAR node otherwise: https://github.com/dotnet/runtime/blob/master/src/coreclr/src/jit/importer.cpp#L2643

However, there are other areas that don't use this method and instead check for GTF_SIDE_EFFECT calling fgInsertCommaFormTemp and calling gtCloneExpr otherwise: https://github.com/dotnet/runtime/blob/master/src/coreclr/src/jit/simd.cpp#L1390-L1397

There are also some places that directly create an additional LCL_VAR node if it needs two copies of the data: https://github.com/dotnet/runtime/blob/master/src/coreclr/src/jit/simd.cpp#L1319

There are also various places that just directly clone or insert a temp without the above associated checks.

category:question
theme:importer
skill-level:intermediate
cost:medium

[tannergooding]

CC. @CarolEidt since I saw these differences when going through simd.cpp

[CarolEidt]

I suspect that these should be unified, though it wouldn't surprise me that some of the differences were due to some varying requirements based on what phase it's in.
@dotnet/jit-contrib - any other insights?

[tannergooding]

I suspect that these should be unified, though it wouldn't surprise me that some of the differences were due to some varying requirements based on what phase it's in.

At least in the case of simd.cpp and hwintrinsic.cpp, it looks to all be during importation. Although, I didn't dig too deeply into all the callsites to validate that was definitely the case.

[EgorBo]

in my PR I used

auto cloned = gtCloneExpr(op1->OperIsLeaf() ? op1 : fgMakeMultiUse(&tree->AsOp()->gtOp1));

(not sure OperIsLeaf() is necessary)

[AndyAyersMS]

impCloneExpr checks for interference of the tree against the IL stack. You need to call this in the importer unless you know for sure you can't have any such interference. It doesn't make sense to call impCloneExpr outside of importation.

[AndyAyersMS]

Given that we haven't had any bugs (that I know of) from the variety of approaches to handling duplicated operands, I'm tempted to say we should put off any cleanup work until after 5.0. We can try to clean up/unify early in 6.0.

Thoughts?

[tannergooding]

That sounds reasonable to me. I was mostly just interested as I had seen a number of varying mechanisms 😄

[AndyAyersMS]

Ok, I'm going to move this.