SROA: allow reconstruction of original aggregate with OPpair #6192

WalterBright · 2016-10-12T06:29:16Z

SROA slices an aggregate into individual variables. Hence, operations needing the original aggregate are disallowed. This PR improves that by allowing loads of the original aggregate by reconstituting it out of the slices using OPpair.

The net result is more cases of SROA can be performed.

andralex · 2016-10-12T23:55:05Z

LGTM - @MartinNowak wanna take a look?

WalterBright · 2016-10-13T03:13:43Z

The FreeBSD 32 failure appears to be a heisenbug timeout.

MartinNowak · 2016-10-14T15:55:28Z

src/backend/gsroa.c

@@ -88,16 +144,30 @@ static void sliceStructs_Replace(SymInfo *sia, elem *e)
                //elem_print(e);
                if (si >= 0 && sia[si].canSlice)
                {
-                    if (e->Eoffset == 0)  // the first slice of the symbol is the same as the original
+                    if (tysize(e->Ety) == 2 * REGSIZE)


Shouldn't you use usePair here?

No, because not every use of the symbol will be 2*REGSIZE, despite usePair.

MartinNowak · 2016-10-14T15:57:53Z

src/backend/gsroa.c

+                if (!sia[si].accessSlice)
+                {
+                    // If never did access it as a slice, don't slice
+                    sia[si].canSlice = false;


Using it this way, doSlice instead of canSlice would better a description.

I prefer can to be answering a question, and do to actually perform the work.

Well, but here it could be sliced but shouldn't be done (b/c it's not used). Anyhow...

isSliced ?

MartinNowak · 2016-10-14T16:02:27Z

src/backend/gsroa.c

                sold->Stype->Tcount++;
-                snew->Stype = type_fake(TYnptr);
+                snew->Stype = type_fake(sia[si].ty1);


Aren't ty0 and ty1 the types of the fields? Why would you use a different type for the replacement symbol?

Because they may be different types. a.ptr and a.length are different, for example.

The confusing part here was that you're reusing sold for the new replacement symbol.

just changing its type

MartinNowak · 2016-10-14T16:05:16Z

src/backend/gsroa.c

+
+                        /* OPpair cannot handle XMM registers, cdpair() and fixresult()
+                         */
+                        if (tyfloating(sia[si].ty0) || tyfloating(sia[si].ty1))


This branch is a bit confusing b/c it seems to rely on the fact that the else if branch below was run for e->E1 and e->E2 beforehand.
Would be simpler to understand if you changed the order of the branches and added a small comment.

Also notice that the condition at https://github.com/dlang/dmd/pull/6192/files#diff-22b8e2c888085072a0c7f94f80991c4eR81 does the same thing as the test here.

No, it doesn't rely on that at all. E1 and E2 could not have been run beforehand for OPvar, because they don't exist for OPvar elems.

So what's really happening here is that a symbol could be used as fat value, but also could get sliced for field access.
Therefor you have to mutually account for the other case.
I'll comment that to make it understandable in the code.

It does account for the other case. I don't really know what you want to say there - the code is there in the other case.

Yes, it just wasn't obvious how the two branches relate to each other.
They are independent b/c different parts of the expression tree can access the same symbol, one as slice, one as fat value using OPpair.

Could have been simpler to later (during slicing) check for usePair && accessSlice && (tyfloating(ty0) || tyfloating(ty1)) instead of mutating canSlice here. But I guess your XMM OPpair work obsoletes this.

I need to do OPpair improvements anyway, and am doing so, but I wanted to keep this PR focussed. Once OPpair is working properly, I'll come back and remove this hack.

Thanks, just wasted 3 days debugging a memory corruption based on this :/.
Wasn't too well put here, but as mentioned ty0 and ty1 are still uninitialized if access as fat value happens before any access to a sliced field.

MartinNowak · 2016-10-14T20:55:54Z

src/backend/gsroa.c

+                            sia[si].ty0 = tybasic(e->Ety);
+                        else
+                            sia[si].ty1 = tybasic(e->Ety);
+                        if (sia[si].usePair && (tyfloating(sia[si].ty0) || tyfloating(sia[si].ty1)))


This is weird, when usePair gets set above (if (sz == 2 * REGSIZE)), it expects ty0/1 to already be set. But here where the ty0/1 assignment is done, you're checking for usePair to be set earlier.
How can both cases be valid?

ty0/1 are defaulted to TYnptr. They are set to correct values when a slice case is encountered. If no slice cases are encountered, then slicing is not done.

MartinNowak · 2016-10-16T10:29:28Z

Auto-merge toggled on

MartinNowak · 2016-10-16T10:29:38Z

Added the comment

WalterBright · 2016-10-16T19:59:35Z

Thanks, @MartinNowak

WalterBright force-pushed the sroa2 branch 6 times, most recently from 5a84458 to 34fd345 Compare October 12, 2016 21:47

andralex assigned MartinNowak Oct 12, 2016

SROA: allow reconstruction of original aggregate with OPpair

0748ff5

WalterBright force-pushed the sroa2 branch from 34fd345 to 0748ff5 Compare October 13, 2016 10:44

MartinNowak reviewed Oct 14, 2016

View reviewed changes

add comment for mutual OPpair/tyfloating check

ac9beec

MartinNowak merged commit 8699245 into dlang:master Oct 16, 2016

WalterBright deleted the sroa2 branch October 16, 2016 19:59

MartinNowak mentioned this pull request Sep 25, 2017

fix PR 7150 - reproducible builds #7153

Merged

stefan-koch-sociomantic mentioned this pull request Oct 30, 2018

Workaround optimizer bug sociomantic-tsunami/ocean#656

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SROA: allow reconstruction of original aggregate with OPpair #6192

SROA: allow reconstruction of original aggregate with OPpair #6192

WalterBright commented Oct 12, 2016

andralex commented Oct 12, 2016

WalterBright commented Oct 13, 2016

MartinNowak Oct 14, 2016

WalterBright Oct 14, 2016

MartinNowak Oct 14, 2016

WalterBright Oct 14, 2016

MartinNowak Oct 14, 2016

stefan-koch-sociomantic Sep 26, 2017

MartinNowak Oct 14, 2016

WalterBright Oct 14, 2016

MartinNowak Oct 14, 2016

WalterBright Oct 16, 2016

MartinNowak Oct 14, 2016

MartinNowak Oct 14, 2016

WalterBright Oct 14, 2016

MartinNowak Oct 16, 2016 •

edited

Loading

WalterBright Oct 16, 2016

MartinNowak Oct 16, 2016

MartinNowak Oct 16, 2016

WalterBright Oct 16, 2016

MartinNowak Sep 24, 2017 •

edited

Loading

MartinNowak Oct 14, 2016

WalterBright Oct 15, 2016

MartinNowak commented Oct 16, 2016

MartinNowak commented Oct 16, 2016

WalterBright commented Oct 16, 2016

SROA: allow reconstruction of original aggregate with OPpair #6192

SROA: allow reconstruction of original aggregate with OPpair #6192

Conversation

WalterBright commented Oct 12, 2016

andralex commented Oct 12, 2016

WalterBright commented Oct 13, 2016

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

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

MartinNowak Oct 16, 2016 • 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

MartinNowak Sep 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MartinNowak commented Oct 16, 2016

MartinNowak commented Oct 16, 2016

WalterBright commented Oct 16, 2016

MartinNowak Oct 16, 2016 •

edited

Loading

MartinNowak Sep 24, 2017 •

edited

Loading