Delete fgArgTabEntry::node by mikedn · Pull Request #24294 · dotnet/coreclr

mikedn · 2019-04-28T18:44:32Z

This replace the GenTree* fgArgTabEntry::node pointer with a pointer to the late arg (use). This way the node pointer can still be obtained from either fgArgTabEntry::use or fgArgTabEntry::lateUse and we no longer need to special case operand replacement (with the exception of preserving GTF_LATE_ARG).

Contributes to #26307

The extra Use for the this arg does increase memory usage a bit, by around 0.05%. I think it's definitely worth it. And we could recover some of that if we eventually move the this arg to the normal arg list so that GenTreeCall (and all large nodes) becomes one pointer smaller.

No diffs. (crossgen/PMI x86/x64 and altjit arm32/arm64)

mikedn · 2019-04-29T17:05:42Z

@CarolEidt

It would be great if we eliminated the need for the fgArgInfo to capture the tree node, though I suspect that's going to be quite a long lead-time task.

Et voila, poof goes fgArgTabEntry::node. Replaced by a pointer to the list node in gtCallLateArgs. Well, it's early stages and it's built on top of other changes (some loop hoisting fix and GT_LIST removal). But anyway, it's doable.

BruceForstall · 2019-09-23T05:02:44Z

cc @sandreenko

erozenfeld · 2019-09-25T23:54:56Z

@dotnet/jit-contrib

mikedn · 2019-09-26T17:46:33Z

While removing fgArgTabEntry::node I transformed some of the assignments to it into asserts that check that it is indeed updated correctly. Initially I wanted to remove them once CI passes and I run all diffs but I think I'm going to keep these asserts for a while, until more call data structures cleanup is done.

It may seem like the asserts are completly useless since it is obviously impossible for node to not be updated - it simply no longer exists. However, there can be some confusion between late and normal args and at least some of those asserts could help prevent issues arising from this.

Ideally this "late arg" nonsense should just go away. Remains to be seen when I'll get to it, I'm also trying to deal with struct related issues and that seems more useful at the moment. Well, at least until I try to do some struct related work and run into all this nonsense again.

sandreenko

Overal look great, I like how simpler this part looks now.

No diffs. (crossgen/PMI x86/x64 and altjit arm32/arm64)

👍
Do you have PIN data?

sandreenko · 2019-09-27T19:35:45Z

            }

-            if (!Compare(op1->AsCall()->gtCallObjp, op2->AsCall()->gtCallObjp))
+            if ((op1->AsCall()->gtCallThisArg != nullptr) != (op2->AsCall()->gtCallThisArg != nullptr))


Nit: I would create a new block under case GT_CALL: and create locals for op1->gtCall, op1->AsCall()->gtCallThisArg, op2->AsCall()->gtCallThisArg.

sandreenko · 2019-09-27T20:09:02Z

    GenTree* GetArgNode(unsigned argIndex)
    {
-        return GetArgEntry(argIndex)->node;
+        return GetArgEntry(argIndex)->GetNode();


a random thing:
in GenTreeCall::Use::SetNode we check that assert(node != nullptr);, so as I understand our contract is that if we have a Use then we have a not-null node, but we don't check that in the Use constructor, so we can call GenTreeCall::Use(nullptr) and do bad things.

Right, I need to check this. AFAIR I didn't add an assert in the Use constructor due to some funny code in fgArgInfo::fgArgInfo that was manufacturing uses out of thin air. That code is gone now so perhaps I can add an assert.

sandreenko · 2019-09-27T20:32:36Z

-            oldArgs   = oldArgs->GetNext();
+            for (unsigned i = 0; i < argTableSize; i++)
+            {
+                if (argTable[i]->use == &(*oldUse))


How much time do we spend in this quadratic loop?

Nit: I do not like &(*oldUse) with hiding castings.

Luckily cloning calls isn't exactly common. The only case I know of is loop hoisiting and that only works with a few "pure" helpers that have 2 arguments.

I don't like &(*oldUse) but I don't understand what do you mean by hiding casts. The only way to avoid this seems to be to avoid using iterators but I'd like to avoid relying on the fact that the arg list is a linked list. It should be an array and using iterators makes changing it easier. Assuming we ever figure out how to deal with the few cases that require as to insert new arguments into this arg list...

oldUse's type is UseIterator ,
*oldUse's type is 'Use&' (because of operator overload Use& operator*() const),
'&(*oldUse)'s type is Use*.

with inlining, it looks like &*(oldUse->m_use), maybe we can add Use* UseIterator::GetUse() const and call it here and below?

mikedn · 2019-09-27T21:07:45Z

Do you have PIN data?

PIN shows no diff (-/+ 0.01%), that's why I didn't mention it. I suppose one would have expected a small improvement due to the simplification of ReplaceCallOperand but either it's not called enough to matter or the addition of an extra use for this negated the improvement.

For fun here's the PIN data: https://1drv.ms/x/s!Av4baJYSo5pjgtFn8rufpZgHaeRGog?e=I5hlUc

The main reason I keep using PIN is that it can actually produce a number like 0.00% 😄. From time to time I compare an unmodified JIT and I've never seen diffs larger than 0.01%.

sandreenko

LGTM

mikedn force-pushed the no-call-list branch 2 times, most recently from be5e3c7 to f1d4104 Compare April 29, 2019 17:00

mikedn force-pushed the no-call-list branch 3 times, most recently from eb2e4f9 to 09978d3 Compare May 3, 2019 19:53

mikedn force-pushed the no-call-list branch 3 times, most recently from e771834 to c727571 Compare May 14, 2019 16:49

jkotas added the area-CodeGen label May 23, 2019

mikedn force-pushed the no-call-list branch from c727571 to b08ef76 Compare July 10, 2019 18:03

mikedn changed the title ~~[WIP] Stop using LIST nodes in GenTreeCall~~ [WIP] [Experimental] GenTreeCall refactoring Aug 23, 2019

mikedn force-pushed the no-call-list branch 2 times, most recently from 20069b6 to 67a9399 Compare September 21, 2019 15:13

mikedn changed the title ~~[WIP] [Experimental] GenTreeCall refactoring~~ [WIP] Delete GenTreeCall::ReplaceCallOperand Sep 21, 2019

mikedn mentioned this pull request Sep 24, 2019

Stop using LIST nodes for FIELD_LIST #26800

Merged

mikedn changed the title ~~[WIP] Delete GenTreeCall::ReplaceCallOperand~~ [WIP] Delete fgArgTabEntry::node Sep 26, 2019

mikedn added 2 commits September 26, 2019 08:55

Store the this call arg in the GenTreeCall::Use

02ea463

Store gtCallThisArg in fgArgTabEntry

43b5dc4

mikedn force-pushed the no-call-list branch from 67a9399 to 47c5991 Compare September 26, 2019 06:01

mikedn added 2 commits September 26, 2019 20:36

Do not store the node in fgArgTabEntry

765ea86

Cleanup fgArgInfo "copy constructor"

dd0adb3

mikedn force-pushed the no-call-list branch from 47c5991 to dd0adb3 Compare September 26, 2019 17:36

mikedn changed the title ~~[WIP] Delete fgArgTabEntry::node~~ Delete fgArgTabEntry::node Sep 26, 2019

sandreenko reviewed Sep 27, 2019

View reviewed changes

Extract call compare code to a separate function

106946d

mikedn added 2 commits September 28, 2019 01:40

Extra asserts

843b2d2

Add UseIterator::GetUse()

8acfa36

sandreenko approved these changes Sep 27, 2019

View reviewed changes

sandreenko merged commit afb9ca8 into dotnet:master Sep 30, 2019

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

sandreenko mentioned this pull request Nov 7, 2019

[WIP/Test pr] Fix GTF_ASG flag on call nodes. #27732

Closed

Conversation

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

Uh oh!

Uh oh!

mikedn commented Apr 29, 2019

Uh oh!

BruceForstall commented Sep 23, 2019

Uh oh!

erozenfeld commented Sep 25, 2019

Uh oh!

mikedn commented Sep 26, 2019

Uh oh!

sandreenko left a comment

Choose a reason for hiding this comment

Uh oh!

sandreenko Sep 27, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sandreenko Sep 27, 2019

Choose a reason for hiding this comment

Uh oh!

mikedn Sep 27, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sandreenko Sep 27, 2019

Choose a reason for hiding this comment

Uh oh!

mikedn Sep 27, 2019

Choose a reason for hiding this comment

Uh oh!

sandreenko Sep 27, 2019

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

sandreenko left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mikedn commented Apr 28, 2019 •

edited

Loading

mikedn commented Sep 27, 2019 •

edited

Loading