add backend/gslice.d: struct slice optimization #6176

WalterBright · 2016-10-06T05:12:59Z

This enables 'slicing' a two register wide aggregate into two register-sized variables, enabling much better enregistering. This is just a start for this. I'm doing this in smaller steps to make it easier to track down causes of any regressions.

WalterBright · 2016-10-06T05:14:59Z

Currently blocked by #6174 in that the optimizations will not be enabled until 6174 is pulled.

WalterBright · 2016-10-06T05:21:25Z

Given the code:

void foo(int[] a, int[] b, int[] c) {
    foreach (i; 0 .. a.length)
        a[i] = b[i] + c[i];
}

the inner loop currently compiles to:

LA:             mov     EAX,018h[ESP]
                mov     EDX,010h[ESP]
                mov     ECX,[EBX*4][EAX]
                add     ECX,[EBX*4][EDX]
                mov     ESI,020h[ESP]
                mov     [EBX*4][ESI],ECX
                inc     EBX
                cmp     EBX,01Ch[ESP]
                jb      LA

with this PR:

L1A:            mov     ECX,[EBX*4][EDI]
                add     ECX,[EBX*4][ESI]
                mov     0[EBX*4][EBP],ECX
                inc     EBX
                cmp     EBX,EDX
                jb      L1A

andralex

ok modulo some nits

andralex · 2016-10-07T01:22:59Z

src/backend/gslice.c

+                    else
+                    {
+                        sia[si].canSlice = false;
+                    }


The complement is actually easier to write and read:

if (tysize(e->Ety) != REGSIZE || e->Eoffset != 0 && e->Eoffset != REGSIZE) { sia[si].canSlice = false; }

andralex · 2016-10-07T01:23:54Z

src/backend/gslice.c

+                }
+                return;
+            }
+            default:


switch with two branches is odd

I expect to add more in the future as I make this optimization more capable.

andralex · 2016-10-07T01:26:52Z

src/backend/gslice.c

+            sliceStructs_Gather(sia, b->Belem);
+    }
+
+    {


so... why this scope here? looks suspect - remove or explain in a comment (I assume it's because of the goto and the variables)

andralex · 2016-10-07T01:27:12Z

src/backend/gslice.c

+                    {
+                        sia[si].canSlice = false;
+                    }
+                }


The complement is actually easier to write and read:

if (tysize(e->Ety) != REGSIZE || e->Eoffset != 0 && e->Eoffset != REGSIZE) { sia[si].canSlice = false; }

andralex · 2016-10-07T01:29:16Z

src/backend/gslice.c

+        for (int si = 0; si < sia_length; si++)
+        {
+            sia2[si + n].canSlice = false;
+            if (sia[si].canSlice)


if (!sia[si].canSlice) { continue; } ... bunch of code ...

I like to avoid things like "not can slice", the negations are cognitively harder.

WalterBright · 2016-10-07T02:04:19Z

Auto-merge toggled on

andralex · 2016-10-07T02:07:08Z

A note to everyone: the new review flow allows one to pull their own request if it was approved. This gives people the opportunity to mind the review nits before merging.

MartinNowak · 2016-10-07T01:48:17Z

src/backend/gslice.c

+{
+    if (debugc) printf("sliceStructs()\n");
+    size_t sia_length = globsym.top;
+    SymInfo *sia = (SymInfo *)malloc(3 * sia_length * sizeof(SymInfo));


Why is it 3? You only seem to be using 2.

Could you use a bitvector for the canSlice part at least.

3 is because it is used for two arrays, sia[] and sia2[]. sia2[] can grow to twice the size of sia[], as symbols can get split into two.

MartinNowak · 2016-10-07T01:55:08Z

src/backend/gslice.c

+    }
+
+    if (!anySlice)
+        goto Ldone;


You're not even using the second part of the sia, nor the si0 in the first part. Looks like a bit vector to me.

I made it a struct so I can add more things to it later. It's very unlikely to get large, as it's the number of local symbols in a function, so size isn't a problem.

MartinNowak · 2016-10-07T02:05:41Z

src/backend/gslice.c

+                sia2[si + n].si0 = si + n;
+                ++n;
+                any = true;
+            }


This body is complex, very low-level, does sth. unexpected (seems like it's replacing the symbol in the global symtab), and comes without explanation/comment. Please add at least a short sentence for the reasoning (what and why), instead of having readers spent 5min. to unpuzzle the low level code, and 10min. on understanding.

MartinNowak · 2016-10-07T02:07:45Z

src/backend/gslice.c

+                snew->Ssymnum = si + n + 1;
+
+                sia2[si + n].canSlice = true;
+                sia2[si + n].si0 = si + n;


FWIW si0 is always the same as the index in sia, so it seems redundant.

It's different, off by n.

Well this is the only place where si0 gets set and it reads sia2[si + n].si0 = si + n;.

MartinNowak · 2016-10-07T02:10:17Z

src/backend/gslice.c

+
+static char __file__[] = __FILE__;      /* for tassert.h                */
+#include        "tassert.h"
+


A brief overview of what this module does and how it does it would be really friendly.

MartinNowak · 2016-10-07T02:14:20Z

src/backend/gslice.c

+                        //printf("replaced with:\n");
+                        //elem_print(e);
+                    }
+                }


No idea what this Eoffset = 0 is trying to achieve.

added comment

MartinNowak · 2016-10-07T02:14:49Z

src/backend/gslice.c

+                    }
+                    else
+                    {
+                        Symbol *s1 = globsym.tab[sia[si].si0 + 1];


The second slice is the next symbol.

WalterBright · 2016-10-07T02:35:28Z

Auto-merge toggled off

WalterBright · 2016-10-07T04:49:03Z

Auto-merge toggled on

MartinNowak · 2016-10-10T06:26:54Z

Ah finally understood what this does, now that I read you use the term slice not only for enregistering slices but for structs in general, I'd highly recommend to use a different term.
With the wide use of the term slice in D, reading this code is just confusing.
How about splitStructs and gsplitstructs.c instead?

dnadlinger · 2016-10-10T06:48:44Z

Regarding names: Is this SROA?

WalterBright · 2016-10-10T07:35:42Z

Yes, it's SROA: http://digital.cs.usu.edu/~allan/AdvComp/Notes/earlyd/

MartinNowak · 2017-02-24T18:20:45Z

According to Digger this caused Issue 17215 – ICE(cgcod.c:findreg) with SIMD and -O -inline.

WalterBright added the Blocked label Oct 6, 2016

WalterBright force-pushed the structslice1 branch from 3b35a15 to e3af5a9 Compare October 6, 2016 05:25

WalterBright changed the title ~~add gslice optimization~~ add backend/gslice.d: struct slice optimization Oct 6, 2016

andralex approved these changes Oct 7, 2016

View reviewed changes

WalterBright force-pushed the structslice1 branch from e3af5a9 to f98bcbe Compare October 7, 2016 01:59

WalterBright removed the Blocked label Oct 7, 2016

MartinNowak reviewed Oct 7, 2016

View reviewed changes

WalterBright force-pushed the structslice1 branch from f98bcbe to de21de2 Compare October 7, 2016 03:17

add gslice optimization

4b842de

WalterBright force-pushed the structslice1 branch from de21de2 to 4b842de Compare October 7, 2016 04:06

WalterBright merged commit 68c35a6 into dlang:master Oct 7, 2016

WalterBright deleted the structslice1 branch October 7, 2016 06:07


		static char __file__[] = __FILE__; /* for tassert.h */
		#include "tassert.h"

add backend/gslice.d: struct slice optimization #6176

add backend/gslice.d: struct slice optimization #6176

Conversation

WalterBright commented Oct 6, 2016

WalterBright commented Oct 6, 2016 • edited

WalterBright commented Oct 6, 2016

andralex 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

andralex Oct 7, 2016 • edited

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

andralex Oct 7, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WalterBright commented Oct 7, 2016

andralex commented Oct 7, 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

MartinNowak Oct 7, 2016 • edited

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

WalterBright commented Oct 7, 2016

WalterBright commented Oct 7, 2016

MartinNowak commented Oct 10, 2016 • edited

dnadlinger commented Oct 10, 2016

WalterBright commented Oct 10, 2016

MartinNowak commented Feb 24, 2017

WalterBright commented Oct 6, 2016 •

edited

andralex Oct 7, 2016 •

edited

andralex Oct 7, 2016 •

edited

MartinNowak Oct 7, 2016 •

edited

MartinNowak commented Oct 10, 2016 •

edited