Re: Filed PR jit/66812 for the code generation issue

[David Malcolm <dmalcolm@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3522 bytes --]

On Wed, 2015-07-08 at 11:05 -0400, David Malcolm wrote:
> On Wed, 2015-07-08 at 10:21 -0400, David Malcolm wrote:
> > On Sat, 2015-07-04 at 16:58 +0100, Dibyendu Majumdar wrote:
> > > On 4 July 2015 at 14:20, Dibyendu Majumdar <mobile@majumdar.org.uk> wrote:
> > > > On 4 July 2015 at 13:11, Dibyendu Majumdar <mobile@majumdar.org.uk> wrote:
> > > >> Looks like in the failure case the code is being incorrectly
> > > >> optimized. I wonder if this is a manifestation of the get_address bug,
> > > >> perhaps the real fix will be better than the patch I am using. I will
> > > >> use the latest gcc 5 branch and see if that helps.
> > > >>
> > > >
> > > > Hi Dave,
> > > >
> > > > I am now using the latest gcc-5-branch from gcc github mirror.
> > > > Unfortunately the issue still persists.
> > > >
> > > > If set optimization level to 0 or 1, then it works ok, but at levels 2
> > > > or 3 the break occurs.
> > > >
> > > 
> > > Adding the -fno-strict-aliasing appears to resolve the issue with -O2
> > > and -O3 but with this enabled the benchmarks are degraded.
> > 
> > I've filed the bad code generation issue as:
> >   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66812
> > 
> > and I'm investigating it.
> 
> 
> Notes on investigation so far.
> 
> With the fixes here
>   https://gcc.gnu.org/ml/jit/2015-q3/msg00025.html
> and here:
>   https://gcc.gnu.org/ml/jit/2015-q3/msg00028.html
> the reproducer from
>  https://gcc.gnu.org/ml/jit/2015-q3/msg00012.html
> now correctly fails with:
>  error: gcc_jit_context_new_field: unknown size for field
> "errorJmp" (type: struct ravi_lua_longjmp)
> 
> Upon applying the equivalent of the fix from:
> https://github.com/dibyendumajumdar/ravi/commit/d65d2e68fbdcf211ed36deea05727f996ede8296
> to the generator you provided, gcc_jit_context_compile completes.
> 
> I don't know if it's generating bad code (given that I don't have Ravi
> itself running, just the reproducer).  However, I tried comparing the
> bug_rdump.txt and bug_rdump_ok.txt reproducers, and tried adding
> -fno-strict-aliasing to the former.
> 
> I turned on GCC_JIT_BOOL_OPTION_DUMP_EVERYTHING and started comparing
> the dumpfiles.
> 
> The diffs stay easily comparable until fake.c.018t.ssa, when numbering
> differences between temporaries mean that a simple "diff" becomes
> unreadable.
> 
> So I wrote a tool to try to alleviate this:
>   https://github.com/davidmalcolm/gcc-dump-diff
> The tool tries to renumber temporaries in dumpfiles so that they are
> more consistent between runs, and then tries to do a textual diff of two
> dumpfiles.
> 
> (note that it might renumber labels also)

Yes, labels *were* being renumbered.  I've fixed that now.

> With this, the dumpfiles become comparable again.  I see that when
> rerunning the bug_rdump.txt reproducer, the first big difference between
> the with and without -fno-strict-aliasing case happens at 035t.fre1.
> 
> Note in the following how, without -fno-strict-aliasing, various
> statements and blocks are eliminated at 035t.fre1, which treats them as
> dead code.  In particular, it seems to have optimized away
> OP_TEST_do_jmp_5_0 and OP_TEST_do_skip_5_0 (if I'm reading things
> right).

It's OP_TEST_do_jmp_5_14, jmp_9_2 and OP_TEST_do_skip_5_15 that are
being optimized away by pass fre1 when -fno-strict-aliasing isn't
supplied.

I'm attaching a fixed diff, showing the correct label names.


> My hunch at this time is that this optimization is being too
> aggressive... for some reason.  I'll continue poking at this.

[snip]

[-- Attachment #2: diff-of-035t.fre1.txt --]
[-- Type: text/plain, Size: 5770 bytes --]

--- ../gcc-git-jit-ravi/src/libgccjit-bug_rdump-no-strict-aliasing/fake.c.035t.fre1
+++ ../gcc-git-jit-ravi/src/libgccjit-bug_rdump/fake.c.035t.fre1
@@ -2,6 +2,15 @@
 
 ;; Function ravif2 (ravif2, funcdef_no=0, decl_uid=364, cgraph_uid=0, symbol_order=0)
 
+Merging blocks 8 and 9
+Removing basic block 10
+Removing basic block 11
+Removing basic block 13
+Removing basic block 12
+Removing basic block 22
+Removing basic block 17
+Merging blocks 8 and 14
+Merging blocks 8 and 15
 ravif2 (struct ravi_lua_State * L)
 {
   struct ravi_TValue * base;
@@ -20,7 +29,6 @@
   <unnamed type> comparison_0_8;
   <unnamed type> comparison_0_9;
   <unnamed type> iftmp.1_0;
-  <unnamed type> iftmp.3_0;
   <unnamed type> iftmp.5_0;
   struct ravi_CallInfo * _0;
   struct ravi_TValue * _0;
@@ -28,31 +36,16 @@
   struct ravi_Proto * _0;
   struct ravi_TValue * _1;
   signed long _0;
+  signed int _0;
+  signed int _1;
+  <unnamed type> _0;
+  signed long _1;
+  signed int _1;
+  struct ravi_CallInfo * _2;
+  struct ravi_TValue * _0;
   struct ravi_Proto * _1;
-  struct ravi_TValue * _2;
-  signed int _0;
-  struct ravi_CallInfo * _2;
-  signed int _1;
-  signed int _2;
-  struct ravi_CallInfo * _3;
   signed int _3;
-  signed int _4;
-  <unnamed type> _0;
-  struct ravi_CallInfo * _4;
-  struct ravi_Proto * _2;
   struct ravi_TValue * _3;
-  signed long _0;
-  struct ravi_Proto * _3;
-  struct ravi_TValue * _4;
-  signed int _0;
-  struct ravi_CallInfo * _5;
-  signed int _6;
-  signed int _7;
-  struct ravi_CallInfo * _6;
-  struct ravi_TValue * _0;
-  struct ravi_Proto * _4;
-  signed int _8;
-  struct ravi_TValue * _6;
 
 entry:
   _0 = L_0(D)->ci;
@@ -66,27 +59,22 @@
   _1 = _0->k;
   _0 = _1->value_.i;
   MEM[(struct ravi_TValue *)base_1 + 16B].value_.i = _0;
-  _1 = cl_0->p;
-  _2 = _1->k;
-  _0 = _2->tt_;
+  _0 = _1->tt_;
   MEM[(struct ravi_TValue *)base_1 + 16B].tt_ = _0;
-  _2 = L_0(D)->ci;
-  base_2 = _2->u.l.base;
-  _1 = MEM[(struct ravi_TValue *)base_2 + 16B].tt_;
-  _2 = MEM[(struct ravi_TValue *)base_2 + 16B].value_.b;
-  if (_1 == 0)
+  _1 = MEM[(struct ravi_TValue *)base_1 + 16B].value_.b;
+  if (_0 == 0)
     goto <bb 8>;
   else
     goto <bb 3>;
 
   <bb 3>:
-  if (_1 == 1)
+  if (_0 == 1)
     goto <bb 4>;
   else
     goto <bb 5>;
 
   <bb 4>:
-  if (_2 == 0)
+  if (_1 == 0)
     goto <bb 6>;
   else
     goto <bb 5>;
@@ -104,119 +92,56 @@
 
   <bb 8>:
   # isfalse_0_4_0 = PHI <1(6), 0(7), 1(2)>
-  MEM[(struct ravi_TValue *)base_2 + 16B].value_.b = 0;
-  MEM[(struct ravi_TValue *)base_2 + 16B].tt_ = 1;
-  _3 = L_0(D)->ci;
-  base_3 = _3->u.l.base;
-  _3 = MEM[(struct ravi_TValue *)base_3 + 16B].tt_;
-  _4 = MEM[(struct ravi_TValue *)base_3 + 16B].value_.b;
-  if (_3 == 0)
-    goto <bb 14>;
+  MEM[(struct ravi_TValue *)base_1 + 16B].value_.b = 0;
+  MEM[(struct ravi_TValue *)base_1 + 16B].tt_ = 1;
+  _0 = 0;
+  _1 = _1->value_.i;
+  MEM[(struct ravi_TValue *)base_1 + 16B].value_.i = _1;
+  _1 = _1->tt_;
+  MEM[(struct ravi_TValue *)base_1 + 16B].tt_ = _1;
+  if (_1 == 0)
+    goto <bb 13>;
   else
     goto <bb 9>;
 
   <bb 9>:
-  if (_3 == 1)
+  if (_1 == 1)
+    goto <bb 11>;
+  else
     goto <bb 10>;
-  else
-    goto <bb 11>;
 
   <bb 10>:
-  if (_4 == 0)
-    goto <bb 12>;
-  else
-    goto <bb 11>;
 
   <bb 11>:
+  # iftmp.5_0 = PHI <1(9), 0(10)>
+  if (iftmp.5_0 != 0)
+    goto <bb 13>;
+  else
+    goto <bb 12>;
 
   <bb 12>:
-  # iftmp.3_0 = PHI <1(10), 0(11)>
-  if (iftmp.3_0 != 0)
-    goto <bb 14>;
-  else
-    goto <bb 13>;
 
   <bb 13>:
-
-  <bb 14>:
-  # isfalse_0_10_0 = PHI <1(12), 0(13), 1(8)>
-  _0 = ~isfalse_0_10_0;
-  if (isfalse_0_10_0 != 0)
-    goto <bb 15> (OP_TEST_do_jmp_5_14);
+  # isfalse_0_16_0 = PHI <1(11), 0(12), 1(8)>
+  MEM[(struct ravi_TValue *)base_1 + 16B].tt_ = 1;
+  printf ("OP_RETURN(pc=%d) return %d args", 13, 1);
+  _2 = L_0(D)->ci;
+  base_2 = _2->u.l.base;
+  _0 = base_2 + 32;
+  L_0(D)->top = _0;
+  _1 = cl_0->p;
+  _3 = _1->sizep;
+  if (_3 > 0)
+    goto <bb 14> (OP_RETURN_if_sizep_gt_0_12_23);
   else
-    goto <bb 22> (OP_TEST_do_skip_5_15);
-
-OP_TEST_do_jmp_5_14:
-jmp_9_2:
-  _4 = L_0(D)->ci;
-  base_4 = _4->u.l.base;
-  _2 = cl_0->p;
-  _3 = _2->k;
-  _0 = _3->value_.i;
-  MEM[(struct ravi_TValue *)base_4 + 16B].value_.i = _0;
-  _3 = cl_0->p;
-  _4 = _3->k;
-  _0 = _4->tt_;
-  MEM[(struct ravi_TValue *)base_4 + 16B].tt_ = _0;
-  _5 = L_0(D)->ci;
-  base_5 = _5->u.l.base;
-  _6 = MEM[(struct ravi_TValue *)base_5 + 16B].tt_;
-  _7 = MEM[(struct ravi_TValue *)base_5 + 16B].value_.b;
-  if (_6 == 0)
-    goto <bb 21>;
-  else
-    goto <bb 16>;
-
-  <bb 16>:
-  if (_6 == 1)
-    goto <bb 17>;
-  else
-    goto <bb 18>;
-
-  <bb 17>:
-  if (_7 == 0)
-    goto <bb 19>;
-  else
-    goto <bb 18>;
-
-  <bb 18>:
-
-  <bb 19>:
-  # iftmp.5_0 = PHI <1(17), 0(18)>
-  if (iftmp.5_0 != 0)
-    goto <bb 21>;
-  else
-    goto <bb 20>;
-
-  <bb 20>:
-
-  <bb 21>:
-  # isfalse_0_16_0 = PHI <1(19), 0(20), 1(15)>
-  MEM[(struct ravi_TValue *)base_5 + 16B].value_.b = 0;
-  MEM[(struct ravi_TValue *)base_5 + 16B].tt_ = 1;
-  printf ("OP_RETURN(pc=%d) return %d args", 13, 1);
-  _6 = L_0(D)->ci;
-  base_6 = _6->u.l.base;
-  _0 = base_6 + 32;
-  L_0(D)->top = _0;
-  _4 = cl_0->p;
-  _8 = _4->sizep;
-  if (_8 > 0)
-    goto <bb 23> (OP_RETURN_if_sizep_gt_0_12_23);
-  else
-    goto <bb 24> (OP_RETURN_else_sizep_gt_0_12_24);
-
-OP_TEST_do_skip_5_15:
-  base_3->value_.b = 1;
-  base_3->tt_ = 1;
-  goto <bb 15> (OP_TEST_do_jmp_5_14);
+    goto <bb 15> (OP_RETURN_else_sizep_gt_0_12_24);
 
 OP_RETURN_if_sizep_gt_0_12_23:
-  luaF_close (L_0(D), base_6);
+  luaF_close (L_0(D), base_2);
 
 OP_RETURN_else_sizep_gt_0_12_24:
-  _6 = base_6 + 16;
-  luaD_poscall (L_0(D), _6);
+  _3 = base_2 + 16;
+  luaD_poscall (L_0(D), _3);
   return 1;
 
 }