Enable SIMD for RyuJIT/x86 #8644

BruceForstall · 2016-12-15T02:22:30Z

This change implements support for Vector<long>, handling
SIMDIntrinsicInit, which takes a LONG, and decomposition of
SIMDIntrinsicGetItem, which produces a LONG.

It also enables SIMD, including AVX, by default for RyuJIT/x86.

BruceForstall · 2016-12-15T02:26:38Z

@sivarv @CarolEidt @pgavlin PTAL
cc @dotnet/jit-contrib

mikedn · 2017-01-13T07:54:20Z

src/jit/lsra.cpp

@@ -8616,7 +8616,15 @@ void LinearScan::resolveRegisters()
        printf("-----------------------\n");

        printf("Resolution Candidates: ");
-        dumpConvertedVarSet(compiler, resolutionCandidateVars);
+        if (compiler->lvaCount == 0)


Is this still needed after #8840?

I assume not. I'll have to pull and rebase.

It shouldn't be needed.

BruceForstall · 2017-02-01T00:17:48Z

This is ready to be reviewed again. I've run 44 stress flavors of the SIMD tests locally and those are clean.

@sivarv @CarolEidt PTAL
cc @dotnet/jit-contrib

sivarv · 2017-02-01T00:37:26Z

src/jit/gentree.cpp

+    else if ((op1->OperGet() == GT_OBJ) && (op1->gtOp.gtOp1->OperGet() == GT_ADDR) &&
+             op1->gtOp.gtOp1->gtOp.gtOp1->OperIsLocal())
+    {
+        setLclRelatedToSIMDIntrinsic(op1->gtOp.gtOp1->gtOp.gtOp1);
    }


Looks like this logic is repeated 3 times. How about abstracting it in a routine SetLclRelatedToSMDIntrinsic()?

The caller would have to call

SetLclRelatedToSMDIntrinsic(op1);
if (op2 != nullptr)
{
SetLclRelatedToSMDIntrinsic(op2);
}

sivarv · 2017-02-01T00:39:07Z

src/jit/gentree.cpp

@@ -16885,21 +16884,28 @@ GenTreeSIMD* Compiler::gtNewSIMDNode(
 GenTreeSIMD* Compiler::gtNewSIMDNode(
    var_types type, GenTreePtr op1, GenTreePtr op2, SIMDIntrinsicID simdIntrinsicID, var_types baseType, unsigned size)
 {
-    // TODO-CQ: An operand may be a GT_OBJ(GT_ADDR(GT_LCL_VAR))), in which case it should be
-    // marked lvUsedInSIMDIntrinsic.
    assert(op1 != nullptr);
    if (op1->OperIsLocal())
    {


On line 16871 we are checking whether op1->OperGet() == GT_LCL_VAR whereas here using OperIsLocal(). I think on line 16871 too we should use op1->OperIsLocal()?

I didn't really want to touch that if there was a reason, but with the refactoring you suggested above, I did it.

sivarv · 2017-02-01T00:40:54Z

src/jit/gschecks.cpp

        lvaTable[shadowVar].lvRegStruct = varDsc->lvRegStruct;
+        lvaTable[shadowVar].lvExactSize = varDsc->lvExactSize;


lvaTable[shadowVar].lvExactSize = varDsc->lvExactSize; [](start = 7, length = 55)

This is already under above under #ifdef FEATURE_SIMD.

In fact initializing lvExactSize for shadow vars other than those known to be SIMD has caused a gc-hole.

Why do we need this change, btw?

I see that Pat added the line you refer to long after I added mine (when I started working on this), so it's probably not needed now.

sivarv · 2017-02-01T00:43:48Z

src/jit/lowerxarch.cpp

+                    info->internalFloatCount = 1;
+                    info->setInternalCandidates(lsra, lsra->allSIMDRegs());
+                    info->isInternalRegDelayFree = true;
+                }


else { info->srcCount = 0 } ?

sivarv · 2017-02-01T00:46:04Z

src/jit/lowerxarch.cpp

+    else if (tree->OperGet() == GT_SIMD)
+    {
+        GenTreeSIMD* simdNode = tree->AsSIMD();
+        var_types    baseType = simdNode->gtSIMDBaseType;


var_types baseType = simdNode->gtSIMDBaseType; [](start = 8, length = 49)

Minor: this variable definition could be moved closer to the point of its use in the block starting at line 4518

sivarv · 2017-02-01T00:49:09Z

src/jit/lowerxarch.cpp

+                    {
+                        assert(baseSize == 2);
+                        ZeroOrSignExtnReqd = (baseType == TYP_SHORT);
+                    }


It would be a good idea to add comments to explain the rationale behind this logic.

sivarv · 2017-02-01T00:51:23Z

src/jit/decomposelongs.cpp

+        case SIMDIntrinsicInit:
+        case SIMDIntrinsicInitN:
+        case SIMDIntrinsicInitArray:
+            NYI("SIMDIntrinsicInit*");


NYI("SIMDIntrinsicInit*"); [](start = 12, length = 26)

You might want to file a tracking git issue to address this NYI later

sivarv · 2017-02-01T00:53:40Z

src/jit/decomposelongs.cpp

+// return: GT_LONG(lowResult, highResult)
+//
+// This isn't optimal codegen, since SIMDIntrinsicGetItem sometimes requires
+// temps that could be shared, for example.


This comment is incomplete?

sivarv · 2017-02-01T00:54:55Z

src/jit/decomposelongs.cpp

+    LIR::Use op2(Range(), &simdTree->gtOp.gtOp2, simdTree);
+    unsigned indexTmpVarNum = op2.ReplaceWithLclVar(m_compiler, m_blockWeight);
+    JITDUMP("[DecomposeSimdGetItem]: Saving op2 tree to a temp var:\n");
+    DISPTREERANGE(Range(), op2.Def());


If the index is known to be a constant, we can avoid introducing temps.

If we intend to address it in a separate PR, please file a tracking issue and a TODO comment here.

sivarv · 2017-02-01T00:57:24Z

CarolEidt

LGTM with some (minor, optional) suggestions.

CarolEidt · 2017-02-01T00:23:31Z

src/inc/clrconfigvalues.h

@@ -562,13 +562,13 @@ CONFIG_DWORD_INFO_EX(INTERNAL_JitLoopHoistStats, W("JitLoopHoistStats"), 0, "Dis
 CONFIG_DWORD_INFO_EX(INTERNAL_JitDebugLogLoopCloning, W("JitDebugLogLoopCloning"), 0, "In debug builds log places where loop cloning optimizations are performed on the fast path.", CLRConfig::REGUTIL_default);
 CONFIG_DWORD_INFO_EX(INTERNAL_JitVNMapSelLimit, W("JitVNMapSelLimit"), 0, "If non-zero, assert if # of VNF_MapSelect applications considered reaches this", CLRConfig::REGUTIL_default)
 RETAIL_CONFIG_DWORD_INFO(INTERNAL_JitVNMapSelBudget, W("JitVNMapSelBudget"), 100, "Max # of MapSelect's considered for a particular top-level invocation.")
-#if defined(_TARGET_AMD64_)
+#if defined(_TARGET_AMD64_) || defined(_TARGET_X86_)


Why wouldn't we use TARGET_XARCH here?

The JIT defines _TARGET_XARCH_ but the VM does not.

CarolEidt · 2017-02-01T00:24:29Z

src/jit/decomposelongs.cpp

+        case SIMDIntrinsicInit:
+        case SIMDIntrinsicInitN:
+        case SIMDIntrinsicInitArray:
+            NYI("SIMDIntrinsicInit*");


Is this intended to be a temporary NYI? Perhaps you could open an issue for it (if you haven't already).

Actually, it appears that Init is handled without decomposition. Shouldn't this just default to the noway_assert?

I removed the NYI. At first I thought these would need decomp, but they don't return LONG values, so don't need it.

CarolEidt · 2017-02-01T00:47:45Z

src/jit/decomposelongs.cpp

+//
+// This isn't optimal codegen, since SIMDIntrinsicGetItem sometimes requires
+// temps that could be shared, for example.
+//


Another possible issue to open?

Well, I could open a general, "evaluate x86 simd CQ" item, and/or one specific to long get[i].

CarolEidt · 2017-02-01T01:26:21Z

src/jit/decomposelongs.cpp

+
+    GenTree* simdTmpVar1    = m_compiler->gtNewLclLNode(simdTmpVarNum, simdTree->gtOp.gtOp1->gtType);
+    GenTree* indexTmpVar1   = m_compiler->gtNewLclLNode(indexTmpVarNum, TYP_INT);
+    GenTree* two1           = m_compiler->gtNewIconNode(2, TYP_INT);


Minor suggestion: A comment here would be nice. It took me a moment to recall why we would need a constant 2.

CarolEidt · 2017-02-01T01:35:17Z

tests/src/JIT/SIMD/VectorUtil.cs

@@ -26,7 +26,7 @@ public static bool CheckValue<T>(T value, T expectedValue)
        }
        if (returnVal == false)
        {
-            Console.WriteLine("CheckValue failed for " + expectedValue + " of type " + typeof(T).ToString());
+            Console.WriteLine("CheckValue failed for type " + typeof(T).ToString() + ". Expected: {0} (0x{0:X}), Got: {1} (0x{1:X})", expectedValue, value);


Nice. thanks.

This change implements support for Vector<long>, handling SIMDIntrinsicInit, which takes a LONG, and decomposition of SIMDIntrinsicGetItem, which produces a LONG. It also enables SIMD, including AVX, by default for RyuJIT/x86.

BruceForstall · 2017-02-06T05:24:21Z

@dotnet-bot test Windows_NT x86 Checked jitstress1
@dotnet-bot test Windows_NT x86 Checked jitstress2_jitstressregs0x10
@dotnet-bot test Windows_NT x86 Checked jitstress2_jitstressregs0x80
@dotnet-bot test Windows_NT x86 Checked jitstress2_jitstressregs1
@dotnet-bot test Windows_NT x86 Checked jitstress2_jitstressregs2
@dotnet-bot test Windows_NT x86 Checked jitstress2_jitstressregs3
@dotnet-bot test Windows_NT x86 Checked jitstress2_jitstressregs4
@dotnet-bot test Windows_NT x86 Checked jitstress2_jitstressregs8
@dotnet-bot test Windows_NT x86 Checked jitstress2
@dotnet-bot test Windows_NT x86 Checked jitstressregs0x10
@dotnet-bot test Windows_NT x86 Checked jitstressregs0x80
@dotnet-bot test Windows_NT x86 Checked jitstressregs1
@dotnet-bot test Windows_NT x86 Checked jitstressregs2
@dotnet-bot test Windows_NT x86 Checked jitstressregs3
@dotnet-bot test Windows_NT x86 Checked jitstressregs4
@dotnet-bot test Windows_NT x86 Checked jitstressregs8
@dotnet-bot test Windows_NT x86 Checked minopts
@dotnet-bot test Windows_NT x86 Checked zapdisable

Enable SIMD for RyuJIT/x86 Commit migrated from dotnet/coreclr@d05009a

dnfclas added the cla-already-signed label Dec 15, 2016

BruceForstall force-pushed the SimdLongDecomp branch from c419a84 to 2845988 Compare December 15, 2016 18:37

BruceForstall force-pushed the SimdLongDecomp branch from 2845988 to d783a6c Compare January 9, 2017 02:52

mikedn reviewed Jan 13, 2017

View reviewed changes

BruceForstall force-pushed the SimdLongDecomp branch 2 times, most recently from c15918e to 6216fc4 Compare January 18, 2017 21:42

BruceForstall force-pushed the SimdLongDecomp branch 2 times, most recently from e9a7e57 to 817ec7f Compare February 1, 2017 00:15

sivarv reviewed Feb 1, 2017

View reviewed changes

CarolEidt approved these changes Feb 1, 2017

View reviewed changes

BruceForstall force-pushed the SimdLongDecomp branch from 817ec7f to b38b5ec Compare February 6, 2017 03:16

Enable SIMD for RyuJIT/x86

cacb796

This change implements support for Vector<long>, handling SIMDIntrinsicInit, which takes a LONG, and decomposition of SIMDIntrinsicGetItem, which produces a LONG. It also enables SIMD, including AVX, by default for RyuJIT/x86.

BruceForstall force-pushed the SimdLongDecomp branch from b38b5ec to cacb796 Compare February 6, 2017 05:23

BruceForstall merged commit d05009a into dotnet:master Feb 6, 2017

BruceForstall deleted the SimdLongDecomp branch February 6, 2017 19:26

jashook mentioned this pull request Feb 8, 2017

Fix warning from an unsigned/signed comparison #9413

Merged

karelz modified the milestone: 2.0.0 Aug 28, 2017

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

Merge pull request dotnet/coreclr#8644 from BruceForstall/SimdLongDecomp

349eb35

Enable SIMD for RyuJIT/x86 Commit migrated from dotnet/coreclr@d05009a

		lvaTable[shadowVar].lvRegStruct = varDsc->lvRegStruct;
		lvaTable[shadowVar].lvExactSize = varDsc->lvExactSize;

Enable SIMD for RyuJIT/x86 #8644

Enable SIMD for RyuJIT/x86 #8644

Uh oh!

Conversation

BruceForstall commented Dec 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BruceForstall commented Dec 15, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BruceForstall commented Feb 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sivarv Feb 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sivarv commented Feb 1, 2017

Uh oh!

CarolEidt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BruceForstall commented Feb 6, 2017

Uh oh!

Uh oh!

BruceForstall commented Dec 15, 2016 •

edited

Loading

sivarv Feb 1, 2017 •

edited

Loading