Add IR support for multireg lclVars & intrinsics #34822

CarolEidt · 2020-04-10T16:35:41Z

This modifies the IR to accommodate multiple registers on GenTreeLclVar and the various intrinsics. It doesn't actually enable support for this, but I have experimental work that is showing promise.
I thought it best to get this reviewed first, as it may result in some discussion.

CarolEidt · 2020-04-10T22:33:02Z

@dotnet/jit-contrib PTAL - I'm trying to get an arm build and a device set up to track down those failures (they're not simple test failures, it looks like I've probably broken xunit).
However, in the meantime perhaps you could have a look at the direction of these changes.

CarolEidt · 2020-04-10T22:34:30Z

src/coreclr/src/jit/gentree.h

@@ -3108,12 +3124,193 @@ struct GenTreeLclVarCommon : public GenTreeUnOp
 #endif
 };

+//------------------------------------------------------------------------
+// MultiRegSpillFlags


These same flags were in use by multiple nodes, but with no code sharing. I'm now using them on GenTreeLclVar as well.

CarolEidt · 2020-04-10T22:36:32Z

src/coreclr/src/jit/gentree.h

-            return 0;
-        }
-        return (gtOtherReg == REG_NA || gtOtherReg == REG_STK) ? 1 : 2;
+        return (TypeGet() == TYP_LONG) ? 2 : 1;


This method used to return the number of actual registers, rather than the total count, but that is actually less useful. (It may be that this change is where I'm failing on arm32).

CarolEidt · 2020-04-10T22:37:47Z

src/coreclr/src/jit/gentree.h

+
+public:
+    var_types     gtSIMDBaseType; // SIMD vector base type
+    unsigned char gtSIMDSize;     // SIMD vector size in bytes, use 0 for scalar intrinsics


By consolidating this information on GenTreeJitIntrinsic we can pack it better and it allows us to have a ClassLayout, which is not only useful for the multireg case, but could both streamline and improve our ability to associate the right class handle with SIMD and HW intrinsics.

sandreenko

Looks good in general, a few questions.
Are you planning to apply that design for multi-reg calls or only for intrinsics?

sandreenko · 2020-04-10T22:46:43Z

src/coreclr/src/jit/gentree.h

+#define GTF_VAR_MULTIREG_DEATH1 0x00040000 // GT_LCL_VAR -- The last-use bit for the second register of a multireg lclVar.
+#define GTF_VAR_MULTIREG_DEATH2 0x00080000 // GT_LCL_VAR -- The last-use bit for the third register of a multireg lclVar.
+#define GTF_VAR_MULTIREG_DEATH3 0x00100000 // GT_LCL_VAR -- The last-use bit for the fourth register of a multireg lclVar.
+#define GTF_VAR_MULTIREG_DEATH_MASK (GTF_VAR_MULTIREG_DEATH1 | GTF_VAR_MULTIREG_DEATH2 | GTF_VAR_MULTIREG_DEATH3)


Is the first register controlled with GTF_VAR_DEATH? Won't it be simpler to add GTF_VAR_MULTIREG_DEATH0 or change GTF_VAR_DEATH value to 0x00040000 to do #define GTF_VAR_MULTIREG_DEATH0 GTF_VAR_DEATH?

Yes, the first register is controlled with GTF_VAR_DEATH. It might be better to change the bit used for that - I'd have to check what bits are available.

sandreenko · 2020-04-10T22:47:47Z

src/coreclr/src/jit/gentree.h

+    {
+        return ((gtFlags & GTF_VAR_MULTIREG) != 0);
+    }
+    void ClearMultiReg()


In which situations could we clear that flag?

The flag means that the node represents a multireg localVar, and it is only set when all the fields are register candidates. If that changes, the flag is cleared. This makes it a bit easier to determine the right code path to take without extra checking.

sandreenko · 2020-04-10T22:50:23Z

src/coreclr/src/jit/gentree.cpp

@@ -1449,8 +1449,7 @@ bool GenTree::Compare(GenTree* op1, GenTree* op2, bool swapOK)
                case GT_HWINTRINSIC:
                    if ((op1->AsHWIntrinsic()->gtHWIntrinsicId != op2->AsHWIntrinsic()->gtHWIntrinsicId) ||
                        (op1->AsHWIntrinsic()->gtSIMDBaseType != op2->AsHWIntrinsic()->gtSIMDBaseType) ||
-                        (op1->AsHWIntrinsic()->gtSIMDSize != op2->AsHWIntrinsic()->gtSIMDSize) ||
-                        (op1->AsHWIntrinsic()->gtIndexBaseType != op2->AsHWIntrinsic()->gtIndexBaseType))
+                        (op1->AsHWIntrinsic()->gtSIMDSize != op2->AsHWIntrinsic()->gtSIMDSize))


why can we ignore GetOtherBaseType now?

Oh, good point! We can't.

CarolEidt · 2020-04-10T23:34:11Z

Are you planning to apply that design for multi-reg calls or only for intrinsics?

My plan is actually to implement multi-reg calls and returns first, and then intrinsics. But I had actually started experimenting with the latter, so I thought it made sense to do all the IR changes together.

sandreenko · 2020-04-13T18:46:19Z

src/coreclr/src/jit/gentree.h

+
+    var_types GetOtherBaseType() const
+    {
+        return gtOtherBaseType;


Do we need to check that gtHWIntrinsicId == NI_AVX2_GatherVector128 || gtHWIntrinsicId == NI_AVX2_GatherVector256 or is it legal to call GetOtherBaseType not only for AVX2 Gather* intrinsic?

If we add that assert, we'd have to add a check to the copy method, which I don't think is productive.

Nit: I like that the new fields are private, but I find it confusing that we access gtOtherReg using this method, maybe we can make them separate fields? It won't change the node size because we allocate them as large nodes. Feel free to ignore that comment.

briansull · 2020-04-13T21:00:07Z

src/coreclr/src/jit/gentree.cpp

@@ -722,6 +722,14 @@ int GenTree::GetRegisterDstCount() const
 #endif
    }
 #endif
+
+#if defined(TARGET_XARCH) && defined(FEATURE_HW_INTRINSICS)
+    if (OperIs(GT_HWINTRINSIC))


Shouldn't this be an "else if", instead of an "if"

Actually, IMO they should all be if instead of else if since the previous if clauses all return.

briansull · 2020-04-13T21:02:02Z

src/coreclr/src/jit/gentree.cpp

@@ -5550,6 +5558,47 @@ bool GenTree::OperMayThrow(Compiler* comp)
    return false;
 }

+//-----------------------------------------------------------------------------------
+// GetMultiRegCount: Return the register count for a multi-reg lclVar.
+//


This is a comment header for the function named: GetFieldCount not GetMultiRegCount

Ah yes, thanks. I renamed them but failed to fix the comment.

briansull · 2020-04-13T21:03:02Z

src/coreclr/src/jit/gentree.cpp

+
+//-----------------------------------------------------------------------------------
+// GetRegTypeByIndex: Get a specific register's type, based on regIndex, that is produced
+//                    by this multi-reg node.


This is a comment header for the function named: GetFieldTypeByIndex not GetRegTypeByIndex

briansull · 2020-04-13T22:08:43Z

src/coreclr/src/jit/gentree.h

+#define GTF_VAR_DEATH           GTF_VAR_MULTIREG_DEATH0
+#define GTF_VAR_MULTIREG_DEATH1 0x00040000 // GT_LCL_VAR -- The last-use bit for the second register of a multireg lclVar.
+#define GTF_VAR_MULTIREG_DEATH2 0x00080000 // GT_LCL_VAR -- The last-use bit for the third register of a multireg lclVar.
+#define GTF_VAR_MULTIREG_DEATH3 0x00100000 // GT_LCL_VAR -- The last-use bit for the fourth register of a multireg lclVar.


I don't think that you can use these bits to track the MULTIREG_DEATH info.

Only the upper 8-bits are node specific ,
the lower 24 bits are common for all nodes: (see GTF_COMMON_MASK)
If a particular node needs more that 8 bits of flags, then another approach has to be taken.

Actually, there are unused bits sprinkled about. I will restructure these to ensure that I'm using bits that aren't used by other nodes.

I need a flag bit on LCL_VAR too, see #34827. Hopefully enough to go around.

I've now consolidated the GTF_VAR flags in bits 0xffc00000, and the GTF_COMMON_MASK is 0x0003FFFF after consolidating. So by my reckoning we have 4 bits in 0x003c0000 that are now available.

tannergooding · 2020-04-15T20:25:54Z

From an API perspective, this will work best with (int Quotient, int Remainder) DivRem(int dividend, int divisor), so for APIs where we have already exposed methods like int DivRem(int dividend, int divisor, out int remainder), we will need to expose new APIs and can likely make the previous overload call the new overload, is that correct?

If so, do we expect this will be in for .NET 5 and therefore an API proposal covering existing cases should be opened/tracked? I can think of Math.DivRem, Math.BigMul, and Bmi2.MultiplyNoFlags off the top of my head, but I imagine we have a few more.

CarolEidt · 2020-04-16T00:02:17Z

@tannergooding - I am first working on allowing assignment of multireg calls to enregistered locals. After that I'll work on the intrinsics, though I have an experimental version of Bmi2.MultiplyNoFlags working.

I'm hoping to have all of this supported for .NET 5, so I think it would be a good thing to develop a proposal. I was thinking that I could potentially synthesize a tuple in the importer, so that I could transform the version that takes an out parameter to return the tuple, but it's not easy to synthesize a struct type in the JIT. Adding that would probably be fairly involved. In my experimental implementation I added a Bmi2.MultiplyNoFlags2 and had the version with the out parameter simply call that one, and the code generated was basically the same as if you'd called the Bmi2.MultiplyNoFlags2 version directly (at least for my simple test case).

CarolEidt · 2020-04-16T00:03:06Z

@dotnet/jit-contrib PTAL - I think I've addressed the feedback.

CarolEidt · 2020-04-16T20:35:42Z

@dotnet/jit-contrib ping

AndyAyersMS · 2020-04-16T21:00:05Z

I take it this is a no-diff change?

sandreenko · 2020-04-16T21:07:30Z

src/coreclr/src/jit/gentree.h

+    // Return Value:
+    //    None
+    //
+    void CopyOtherRegFlags(GenTreeLclVar* from)


Does it accept from as lclVar or as a call? The header is probably incorrect.

Ooops, yes, cut and paste error.

sandreenko · 2020-04-16T21:13:41Z

src/coreclr/src/jit/gentree.h

+
+    var_types GetOtherBaseType() const
+    {
+        return gtOtherBaseType;


Nit: I like that the new fields are private, but I find it confusing that we access gtOtherReg using this method, maybe we can make them separate fields? It won't change the node size because we allocate them as large nodes. Feel free to ignore that comment.

sandreenko · 2020-04-16T21:15:17Z

src/coreclr/src/jit/gentree.h

    {
+        gtSIMDIntrinsicID = simdIntrinsicID;
+        assert(gtSIMDIntrinsicID == simdIntrinsicID);


what does this assert check?

It checks that the intrinsic ID fits into the the allocated space. At one time I'd made this smaller, before I restructured a bit. This could now be deleted.
Note, though, that GenTreeHWIntrinsic and GenTreeSIMD which derive from GenTreeJitIntrinsic are small nodes. I think we just allocate a large node for GT_INTRINSIC because it might become a call.

CarolEidt · 2020-04-16T21:29:50Z

I take it this is a no-diff change?

Yes, that was the purpose of #34969 - to separate out the functional changes.

sandreenko

LGTM

CarolEidt added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Apr 10, 2020

CarolEidt commented Apr 10, 2020

View reviewed changes

sandreenko reviewed Apr 10, 2020

View reviewed changes

sandreenko reviewed Apr 13, 2020

View reviewed changes

CarolEidt force-pushed the AddMultiRegNodes branch from 7925cb2 to 9116cdb Compare April 13, 2020 19:22

briansull reviewed Apr 13, 2020

View reviewed changes

CarolEidt mentioned this pull request Apr 14, 2020

Fix some flag uses #34969

Merged

CarolEidt force-pushed the AddMultiRegNodes branch from ec72066 to 3025aaa Compare April 15, 2020 14:26

Add IR support for multireg lclVars & intrinsics

0de1a70

CarolEidt force-pushed the AddMultiRegNodes branch from 3025aaa to 0de1a70 Compare April 15, 2020 20:19

jaredpar mentioned this pull request Apr 15, 2020

Errors installing the SDK during builds #34015

Closed

jaredpar mentioned this pull request Apr 16, 2020

Restore failing on our projects #35057

Closed

sandreenko reviewed Apr 16, 2020

View reviewed changes

PR Feedback

42216f5

sandreenko approved these changes Apr 16, 2020

View reviewed changes

CarolEidt merged commit c2f0bb5 into dotnet:master Apr 17, 2020

CarolEidt deleted the AddMultiRegNodes branch April 28, 2020 16:45

CarolEidt mentioned this pull request Jun 3, 2020

Bmi2.MultiplyNoFlags issues #11782

Open

ghost locked as resolved and limited conversation to collaborators Dec 9, 2020

Add IR support for multireg lclVars & intrinsics #34822

Add IR support for multireg lclVars & intrinsics #34822

Conversation

CarolEidt commented Apr 10, 2020

CarolEidt commented Apr 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sandreenko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CarolEidt commented Apr 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

briansull Apr 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tannergooding commented Apr 15, 2020

CarolEidt commented Apr 16, 2020

CarolEidt commented Apr 16, 2020

CarolEidt commented Apr 16, 2020

AndyAyersMS commented Apr 16, 2020

sandreenko Apr 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CarolEidt commented Apr 16, 2020

sandreenko left a comment

Choose a reason for hiding this comment

briansull Apr 13, 2020 •

edited

Loading

sandreenko Apr 16, 2020 •

edited

Loading