512824 - TM: gcc complains about invalid aliasing in LIR.h

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Andreas Gal :gal]

In file included from ../nanojit/nanojit.h:297,
                 from ../jsbuiltins.h:45,
                 from ../jstracer.h:52,
                 from ../jscntxt.cpp:71:
../nanojit/LIR.h: In member function ‘nanojit::LIns* nanojit::LInsOp0::getLIns()’:
../nanojit/LIR.h:326: warning: type-punning to incomplete type might break strict-aliasing rules
../nanojit/LIR.h: In member function ‘nanojit::LIns* nanojit::LInsOp1::getLIns()’:
../nanojit/LIR.h:341: warning: type-punning to incomplete type might break strict-aliasing rules
../nanojit/LIR.h: In member function ‘nanojit::LIns* nanojit::LInsOp2::getLIns()’:
../nanojit/LIR.h:358: warning: type-punning to incomplete type might break strict-aliasing rules
../nanojit/LIR.h: In member function ‘nanojit::LIns* nanojit::LInsOp3::getLIns()’:
../nanojit/LIR.h:376: warning: type-punning to incomplete type might break strict-aliasing rules
../nanojit/LIR.h: In member function ‘nanojit::LIns* nanojit::LInsLd::getLIns()’:
../nanojit/LIR.h:392: warning: type-punning to incomplete type might break strict-aliasing rules
../nanojit/LIR.h: In member function ‘nanojit::LIns* nanojit::LInsSti::getLIns()’:
../nanojit/LIR.h:410: warning: type-punning to incomplete type might break strict-aliasing rules
../nanojit/LIR.h: In member function ‘nanojit::LIns* nanojit::LInsSk::getLIns()’:
../nanojit/LIR.h:424: warning: type-punning to incomplete type might break strict-aliasing rules
../nanojit/LIR.h: In member function ‘nanojit::LIns* nanojit::LInsC::getLIns()’:
../nanojit/LIR.h:440: warning: type-punning to incomplete type might break strict-aliasing rules
../nanojit/LIR.h: In member function ‘nanojit::LIns* nanojit::LInsP::getLIns()’:
../nanojit/LIR.h:455: warning: type-punning to incomplete type might break strict-aliasing rules
../nanojit/LIR.h: In member function ‘nanojit::LIns* nanojit::LInsI::getLIns()’:
../nanojit/LIR.h:469: warning: type-punning to incomplete type might break strict-aliasing rules
../nanojit/LIR.h: In member function ‘nanojit::LIns* nanojit::LInsI64::getLIns()’:
../nanojit/LIR.h:485: warning: type-punning to incomplete type might break strict-aliasing rules

Comment 1

[Nicholas Nethercote [inactive]]

What GCC are you using?  I don't see it with mine:

  i686-apple-darwin9-gcc-4.0.1 (GCC) 4.0.1 (Apple Inc. build 5493)

I think I can see how to fix it, but I'd need to reproduce it first.

Comment 2

[Andreas Gal :gal]

Yeah, this is apple's gcc 4.2.

Run configure with CC="gcc-4.2" CXX="g++-4.2" ./configure

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: patch

It's 5pm Friday afternoon here so assuming this passes review I probably won't land it until Monday morning.  Andreas, feel free to land it in the meantime if you like.

Comment 4

[Andreas Gal :gal]

I am not convinced this is the way to go. I will play around with this a bit. Maybe a union can save us here with a bit more grace.

Comment 5

[Andreas Gal :gal]

Luke, any comments on this patch and the underlying data structure layout trickery? The structures involved are bottom-aligned (all share the same last word).

Comment 6

[Luke Wagner [:luke]]

I'm not sure I understand all the constraints, but a solution to the constraints I do see would be to (in source order):
 0. declare (but do not define) all the class LInsXYZ
 1. declare and define class LIns, but do not define member functions that rely on the definitions of class LInsXYZ (e.g. all those offsetof uses)
 2. define all the class LInsXYZ
 3. define all the LIns member functions (prefixed with inline of course) pulled from step 1

How does that sound?

Comment 7

[David Mandelin [:dmandelin]]

Option A: Define the conversion to LIns* as a member of LIns:

        template<typename L>
        static LIns* getLIns(L *ins) {
            return (LIns*) &ins->ins;
        }

LIns is defined at this point, so no warning. It does change the API for converting to LIns*, but there seem to be only 10 call sites or so.

Option B: Define the conversion to LIns* as a template function, so it can go before LIns and LInsXYZ, and thus will be visible to LInsXYZ:

        template<typename L>
        inline LIns* toLIns(L *ins) { return (LIns*) &ins->ins; }

LIns will be defined before this is instantiated, so no warning. Now, define the getLIns member functions like this:

        // In LInsSk
        template<typename L> friend LIns* toLIns(L*);
        LIns* getLIns() { return toLIns(this); }

Same API as now.

Comment 8

[Luke Wagner [:luke]]

I didn't say this explicitly, but the idea behind comment 6 was that these casts from LInsXYZ to LIns wouldn't be necessary at all; the LIns would just be a simple member variable of the LInsXYZ.

Comment 9

[David Mandelin [:dmandelin]]

(In reply to comment #8)
> the LIns would just be a simple member variable of the LInsXYZ.

That is a good thing. I had been concerned that separating method definitions from the class definition would inhibit inlining. But I think GCC can inline pretty much anything as long it's in the same compilation, and I think these are all used only in LIR.cpp.

Comment 10

[Luke Wagner [:luke]]

(In reply to comment #9)
> That is a good thing. I had been concerned that separating method definitions
> from the class definition would inhibit inlining. But I think GCC can inline
> pretty much anything as long it's in the same compilation, and I think these
> are all used only in LIR.cpp.

AFAIK inline member definitions are going to be equivalent from a compiler's to out-of-class definitions with 'inline', from a compiler's POV, as long as the out-of-class definition is visible before the point of usage.

Comment 11

[David Mandelin [:dmandelin]]

(In reply to comment #10)
> (In reply to comment #9)
> > That is a good thing. I had been concerned that separating method definitions
> > from the class definition would inhibit inlining. But I think GCC can inline
> > pretty much anything as long it's in the same compilation, and I think these
> > are all used only in LIR.cpp.
> 
> AFAIK inline member definitions are going to be equivalent from a compiler's to
> out-of-class definitions with 'inline', from a compiler's POV, as long as the
> out-of-class definition is visible before the point of usage.

I think a smart compiler is supposed to inline most everything worth inlining no matter where or how it's defined. I did confirm that GCC will inline functions defined in the same file regardless of order or 'inline' keyword at -O3 but not -O2. And I think we currently use -O3. So I think your design is better--using the real type instead of a bunch of casts is a significant advantage in my book.

Comment 12

[Nicholas Nethercote [inactive]]

Attachment: patch v2

This patch takes the approach suggested by Luke in comment 6 and comment 8.
Note that the get/set functions definitions for class LIns are still within
LIR.h; this works because they are all inline.

Since lots of code was getting moved, I took the opportunity to do some
cleanups:
- I put the get/set functions in a sensible order, one that mirrors the
  LInsXYZ order.
- I made some get/set functions inline which previously weren't, and moved
  some of the definitions from LIR.cpp into LIR.h for consistency.  This
  partially addresses bug 514066.
- I added some 'const' qualifiers to those get functions that lacked them.
- I moved callArgN() into class LIns, a better place for it.
- I moved staticSanityCheck() from LIR.h to LIR.cpp because it's big.

Performance-wise, I'm getting a 3--5ms speedup on SunSpider, presumably due
to the inlining -- Cachegrind confirms that the number of instructions
executed has dropped.

But I'm getting a 4-5% slowdown for v8-deltablue which surprised me because
it hardly traces.  Cachegrind says there's a 10% I1-cache miss jump,
hopefully that's just a temporary fluke that will disappear when I update to
a newer revision (but hopefully the SS speedups are more durable).

Comment 13

[Andreas Gal :gal]

Comment on attachment 399151 [details] [diff] [review]
patch v2

Awesome. Thanks.

Comment 14

[Nicholas Nethercote [inactive]]

Ed, Rick, any objections to this please lodge them soon -- otherwise I'll commit this tomorrow morning (Melbourne time).  Thanks.

BTW: Andreas, are you assuming that the deltablue slowdown is a fluke and thus not worth worrying about?

Comment 15

[Andreas Gal :gal]

If it is significant, its unintentional code rearrangement. That would break eventually anyway through some unrelated patch. I can't see how would slow us down in any reasonable way. I am all for not permitting perf regressions, but it has to be tangible and related.

Comment 16

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/tracemonkey/rev/370788804597

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/mozilla-central/rev/370788804597

Comment 18

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/91fb366864da