[JIT] - Fixed sub-optimal optimization for a % 1 to Zero

[TIHan]

Description

Should resolve: #10956

We already did the transformation of a % 1, but we ignored it if op1 had any side effects. This PR should fix that.

X64 Diff:

Acceptance Criteria

• Disasm tests

[ghost]

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

Issue Details

---

** Description **

Should resolve: #10956

Simple optimization. Will transform a % 1 to be 0.

** Acceptance Criteria **

• Disasm tests

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

[SingleAccretion]

Note there is already an optimization that does this:

runtime/src/coreclr/jit/morph.cpp

Lines 9872 to 9887 in bdd67af

	// For "val % 1", return 0 if op1 doesn't have any side effects
	// and we are not in the CSE phase, we cannot discard 'tree'
	// because it may contain CSE expressions that we haven't yet examined.
	//
	if (((op1->gtFlags & GTF_SIDE_EFFECT) == 0) && !optValnumCSE_phase)
	{
	if (op2->IsIntegralConst(1))
	{
	GenTree* zeroNode = gtNewZeroConNode(typ);
	#ifdef DEBUG
	zeroNode->gtDebugFlags |= GTF_DEBUG_NODE_MORPHED;
	#endif
	DEBUG_DESTROY_NODE(tree);
	return zeroNode;
	}
	}

.

[TIHan]

Good catch. Will probably remove it in favor of this since it expands it.

[TIHan]

@dotnet/jit-contrib This is ready.

[jakobbotsch]

/azp run Fuzzlyn

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[jakobbotsch]

The failures look related

[TIHan]

@dotnet/jit-contrib @jakobbotsch This is ready again.

[TIHan]

@dotnet/jit-contrib @EgorBo This is ready for review again. We update the value number for the zero node, and we disable the optimization if we are not in global morph when there are side effects.

[TIHan]

@dotnet/jit-contrib ready again

[TIHan]

@dotnet/jit-contrib @jakobbotsch @EgorBo This is ready again too. Fixed a minor issue when setting the integral value for GT_CNS_LNG on x86/arm.

[TIHan]

@dotnet/jit-contrib This is ready for another round of review, based on our internal discussion yesterday.

[BruceForstall]

One thing I don't remember:

If we have <HasDisasmCheck>true<...>, why do we need to clear TieredCompilation/JITMinOpts? Shouldn't the "HasDisasmCheck" logic do that for us?

[TIHan]

<HasDisasmCheck>true<...> does not force any environment variables that affect codegen - so it is up to the test itself to decide them.