Stop using LIST nodes for FIELD_LIST by mikedn · Pull Request #26800 · dotnet/coreclr

mikedn · 2019-09-20T15:21:23Z

Changes GT_FIELD_LIST to be a special node with a list of "uses" for fields, similar to GT_PHI.

Contributes to #19876

On x64 (which doesn't really use GT_FIELD_LIST) PIN shows a 0.1% improvement. That's probably due to a few rationalizer checks that now only test for GT_LIST. It would probably be more interesting to see results for x86 but I couldn't get PIN to work on x86. On x86 there's a tiny drop in memory usage (0.03% or something like that).

mikedn · 2019-09-24T18:18:37Z

@sandreenko Does it sound reasonable to remove GenTreeCall::regArgList at the same time fgArgTabEntry::node is removed? AFAIR you once tried to remove regArgList but restored it because it was useful in catching node update failures?

sandreenko · 2019-09-24T18:34:51Z

@sandreenko Does it sound reasonable to remove GenTreeCall::regArgList at the same time fgArgTabEntry::node is removed?

Yes, feel free to remove that.

AFAIR you once tried to remove regArgList but restored it because it was useful in catching node update failures?

Yes, also there were noway asserts and buggy code in assertion propagation that used it for correctness, that was cleaned. So now regArgList is debug only.

mikedn · 2019-09-24T18:39:52Z

Thanks! (and oops, I asked the question in the wrong PR, it should have been in #24294)

erozenfeld · 2019-09-25T23:42:17Z

@dotnet/jit-contrib

CarolEidt

Just comments and questions at this point, but I really love this change - I've never been a fan of the FIELD_LIST representation; I think it was one of the worst list cases.

CarolEidt · 2019-09-30T15:42:39Z

-            (void)new (comp, GT_FIELD_LIST) GenTreeFieldList(argHi, 4, TYP_INT, fieldList);
+            GenTreeFieldList* fieldList = new (comp, GT_FIELD_LIST) GenTreeFieldList();
+            fieldList->gtUses           = new (comp, CMK_ASTNode) GenTreeFieldList::Use(argLo, 0, TYP_INT);
+            fieldList->gtUses->SetNext(new (comp, CMK_ASTNode) GenTreeFieldList::Use(argHi, 4, TYP_INT));


Did you consider adding a method to GenTreeFieldList to add fields?

Yes, every time I did one of these "stop using GT_LIST" changes. The problem is that it's kind of risky to have something like "AddUse" because we only keep track of the head of the linked list and appending would require traversing the list to find the tail. I think there are 2 ways to handle this:

add something similar to gtNewCallArgs - gtNewFieldList or something like that - that hides away this manual list manipulation in those cases where the number of fields is statically known

add something like UseListBuilder - a small class that keeps track of both the head and tail and can be used in the rest of the cases.

This should cover almost all cases. AFAIR the only place where we still need to do some manual linked list manipulation in long decomposition (and that's the main reason I maintained the existing linked list approach, otherwise arrays would be a very tempting alternative).

Right. Another option would be to have constructor overloads for the known-number-of-fields cases. But I'm OK with deferring consideration of such a change.

Though in the case of field list (and PHIs) I don't think there's anything stopping us from adding the tail of the list to the node, there's enough space in GenTreeFieldList for that. It's only calls that are problematic, they're large enough even without adding more pointers to them.

Well, I added AddField and InsertField. Not sure I like all this too much, in part due to the side effect inheritance. Sure, GenTreeArgList did this automatically (and so do other GenTree derived classes). And I now realize that this happens even in LIR, where side effects are not supposed to be inherited. So I also added AddFieldLIR and InsertFieldLIR to deal with this case.

Opinions?

@sandreenko too

CarolEidt · 2019-09-30T15:51:04Z

+                    GenTreeFieldList::Use(argx->gtOp.gtOp2, OFFSETOF__CORINFO_TypedReference__type, TYP_I_IMPL));
+            fieldList->gtFlags |=
+                (fieldList->gtUses->GetNode()->gtFlags | fieldList->gtUses->GetNext()->GetNode()->gtFlags) &
+                GTF_ALL_EFFECT;


This flag inheritance was previously done by the GenTreeArgList constructor, and that's generally how it's done. It seems like it might be better to have an AddField method on GenTreeFieldList to take care of this.

CarolEidt · 2019-09-30T15:53:56Z

+    noway_assert(newArg->OperIs(GT_FIELD_LIST));

    // We need to propagate any GTF_ALL_EFFECT flags from the end of the list back to the beginning.
    // This is verified in fgDebugCheckFlags().


This comment could be updated; we only need to update the FIELD_LIST node, where previously we actually had to propagate the flags up.

mikedn · 2019-10-01T19:47:17Z

-
-            // First, insert the field node into the sorted list.
-            GenTreeFieldList* prev = nullptr;
-            for (GenTreeFieldList* cursor = head;; cursor = cursor->Rest())


Sorting code was broken, it never updated prev. The reason why it worked is that the field list was already sorted by increasing offset so the bug here effectively turned this code into list reversal and we ended up getting the expected order. I don't see any reason to create field list with a different initial order so it seems preferable to just reverse the list and avoid the quadratic complexity concerns.

Wow - glad you got this "sorted" ;-)

CarolEidt

This looks great now; thanks for the updates. Ping me when you're ready to remove the [WIP] and we can trigger the failing CI tests (for now, I believe there are still issues in the queue).

mikedn · 2019-10-02T16:36:31Z

@CarolEidt Thanks, I've run diffs again (none) and done another pass checking for possible fallout from the change to TYP_STRUCT. I think this is done now.

Off to finish the #21711 monster 😁

sandreenko · 2019-10-02T17:18:45Z

It would probably be more interesting to see results for x86 but I couldn't get PIN to work on x86. On x86 there's a tiny drop in memory usage (0.03% or something like that).

PIN should work on x86, just run in from SysWOW64 cmd ("C:\Windows\SysWOW64\cmd.exe"), if it doesn't work then I will measure the PIN numbers for this change later today.

mikedn · 2019-10-02T17:24:16Z

@sandreenko AFAIR the problem I had with pin on x86 was that I couldn't get it to compile. If that worked for you I can try again one day. Thanks for the offer to run it!

sandreenko

LGTM in general.

a few comments/questions.

PIN for x86 (release, no pgo, 5 runs average):
base: 22057891557,
diff: 22021891998,
improvement is 0.1%.

sandreenko · 2019-10-02T17:23:13Z

+            use = use->GetNext();
+        }
+
+        bool operator==(const UseIterator& other)


these operators should be const, shouldn't they?

Though given that it took 2 retries to get CI clean it can probably wait until the next GT_LIST related PR.

sandreenko · 2019-10-02T18:02:20Z

+    //
+    static bool Equals(GenTreeFieldList* list1, GenTreeFieldList* list2)
+    {
+        // TODO-Cleanup: The list type is actually ignored. It should always be TYP_STRUCT


what about gtFlags? I understand that it will be checked in the loop, but it could be a fast exit if they are not equal.

Currently this is only called from Compare and that does its own flag check already. That check is kind of weird:

coreclr/src/jit/gentree.cpp

Lines 1248 to 1252 in f777668

/* Sensible flags must be equal */

if ((op1->gtFlags & (GTF_UNSIGNED)) != (op2->gtFlags & (GTF_UNSIGNED)))

{

return false;

}

Even if we want to be able to use Equals directly (IMO it would be a reasonable thing to do) it's not clear what flags should be checked.

sandreenko · 2019-10-02T18:06:01Z

+
+    if (src->OperIs(GT_FIELD_LIST))
    {
+#ifdef _TARGET_X86_


Why the condition and #ifdef were switched?
so now x64 returns when it sees GT_FIELD_LIST in this method.

Side effect of changing the field list type to TYP_STRUCT from TYP_INT or whatever happens to be the first field type.

With the original ifdef code was reaching an if (src->TypeGet() != TYP_STRUCT) that's below. Inside that if the only thing that happens is an attempt to contain constant operands so in the case of FIELD_LIST nothing really happened.

With the change to TYP_STRUCT, that if is no longer entered and instead some code that's expecting an OBJ is reached.

So it was always the case that FIELD_LIST was supposed to be ignored on x64 but the way this worked was a bit "by accident".

Thanks for the explanation!

mikedn · 2019-10-03T05:32:57Z

PIN for x86 (release, no pgo, 5 runs average):
base: 22057891557,
diff: 22021891998,
improvement is 0.1%

Thanks! It's interesting that PIN shows the same improvement as on x64. So it's very likely the improvements are indeed due to the removed field list checks rather than any field list handling code changes. I'm curious if we'll get anything more out of it when I remove the final GT_LIST usage - SIMD/HWIntrinsic nodes.

sandreenko

LGTM, thank you for that change!

* Stop using LIST nodes for FIELD_LIST * Change GT_FIELD_LIST type to TYP_STRUCT * Smaller GenTreeField::Use * Delete out of date comment * Cleanup LowerArg duplicated code * Add AddField/InsertField

mikedn force-pushed the no-list-field branch 2 times, most recently from ee461d8 to 199b6f9 Compare September 21, 2019 07:49

sandreenko added the area-CodeGen label Sep 23, 2019

mikedn force-pushed the no-list-field branch from 199b6f9 to 5e61a01 Compare September 30, 2019 05:41

CarolEidt reviewed Sep 30, 2019

View reviewed changes

mikedn added 2 commits October 1, 2019 11:42

Stop using LIST nodes for FIELD_LIST

ebdb551

Change GT_FIELD_LIST type to TYP_STRUCT

4dc7a42

mikedn force-pushed the no-list-field branch from 5e61a01 to 4459aef Compare October 1, 2019 19:05

mikedn added 3 commits October 1, 2019 22:06

Smaller GenTreeField::Use

fcc023d

Delete out of date comment

bf6b7e0

Cleanup LowerArg duplicated code

8b33021

mikedn force-pushed the no-list-field branch from 4459aef to 1b2a09a Compare October 1, 2019 19:06

Add AddField/InsertField

809f790

mikedn force-pushed the no-list-field branch from 1b2a09a to 809f790 Compare October 1, 2019 19:29

mikedn commented Oct 1, 2019

View reviewed changes

CarolEidt approved these changes Oct 2, 2019

View reviewed changes

mikedn changed the title ~~[WIP] Stop using LIST nodes for FIELD_LIST~~ Stop using LIST nodes for FIELD_LIST Oct 2, 2019

sandreenko reviewed Oct 2, 2019

View reviewed changes

sandreenko approved these changes Oct 3, 2019

View reviewed changes

sandreenko merged commit 0a00ee7 into dotnet:master Oct 3, 2019

mikedn deleted the no-list-field branch October 5, 2019 07:12

	/* Sensible flags must be equal */
	if ((op1->gtFlags & (GTF_UNSIGNED)) != (op2->gtFlags & (GTF_UNSIGNED)))
	{
	return false;
	}

Conversation

mikedn commented Sep 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mikedn commented Sep 24, 2019

Uh oh!

sandreenko commented Sep 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mikedn commented Sep 24, 2019

Uh oh!

erozenfeld commented Sep 25, 2019

Uh oh!

CarolEidt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CarolEidt left a comment

Choose a reason for hiding this comment

Uh oh!

mikedn commented Oct 2, 2019

Uh oh!

sandreenko commented Oct 2, 2019

Uh oh!

mikedn commented Oct 2, 2019

Uh oh!

sandreenko 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!

mikedn commented Oct 3, 2019

Uh oh!

sandreenko left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mikedn commented Sep 20, 2019 •

edited

Loading

sandreenko commented Sep 24, 2019 •

edited

Loading