Re: [INTERNALS-WIN] Re: [PHP-DEV] Windows (Visual Studio) compiler stuff

Hi Anatol,

----- Original Message -----
From: "Anatol Belski"
Sent: Wednesday, November 25, 2015

> Hi Matt,
>
>> -----Original Message-----
>> From: Matt Wilmas [mailto:[email protected]]
>> Sent: Monday, November 23, 2015 8:15 AM
>> To: Anatol Belski <[email protected]>; [email protected];
> internals-
>> [email protected]
>> Cc: 'Dmitry Stogov' <[email protected]>; 'Pierre Joye'
> <[email protected]>
>> Subject: Re: [INTERNALS-WIN] Re: [PHP-DEV] Windows (Visual Studio)
> compiler
>> stuff
>>
>> Hi Anatol, all,
>>
>> ----- Original Message -----
>> From: "Anatol Belski"
>> Sent: Monday, November 16, 2015
>>
>> [...]
>>
>> noinline did have an effect -- 12 KB smaller php7.dll.  So, obviously 
>> it's
>> preventing those zend_never_inline functions from being inlined when they
>> currently are.  Dmitry surely had reason to make them that way --
> cache-related,
>> I assume.  Any difference, however "minor," is the same as other
> compilers, so
>> it's nice to know this can be used, with so many of the other GCC/Clang
> "tricks"
>> missing...
>>
> I wasn't telling it wouldn't work. We should check for possible
> implications. If there's nothing negative, so we can add this into master.
> It always depends, smaller image size vs. function call.

It works and I don't see how there could be implications, so "do it" I say. 
;-)

It doesn't really depend in the case of zend_never_inline -- the *point* is 
to ensure a function call (and smaller code size).  Question/take it up with 
Dmitry otherwise. :-P

>> BTW, something "big" not getting inlined even when forced?  I know the
> "rules"
>> about what can't be [force] inlined (basically same as GCC) and size 
>> isn't
> one of
>> them. :-)  (I hope not.)  As I've mentioned a bit, to be seen soon, my
> "compile-
>> time" param parsing optimization will have the "hugest"
>> inline function, but it compiles down to literally nothing, which I
> finally got to
>> work with MSVC as well.  That's why I wasn't liking the idea of a
> standalone copy
>> of that stuff adding several KB to each module...
>>
> Size is one of the factors, the concrete code and usage, too. Despite 
> that,
> any compiler doc says that inline is just a suggestion.

This is unrelated to anything anyway, but...  We're not talking about "just 
inline" here, but always/force.  Much more than "just a suggestion."  At 
least when optimization is enabled, it WILL be inlined provided it doesn't 
contain one of the things that makes it ineligible for inlining.

So it's more like Arnold in T2: "I insist."  Or, a very strong suggestion.

>> > I guess I've understood what you're talking about - abut unreferenced
>> > COMDATs (or maybe also duplicated COMDATs). There is a variety of
>> > situations for that, not possibly only inlining. Fixing it is done in
>> > PHP when building with --enable-debug-pack, that is on in release
>> > builds. In your experiments, if you add /Zi CFLAG (or explicitly /Gy)
>> > and /OPT:REF,ICF LDFLAG - that will solve it for yur other project.
>> > You can read more about COMDAT on MSDN.
>>
>> Yeah, I know about the COMDAT stuff.  And I thought I had tried the
> /OPT:REF,
>> etc. on a standalone test a while ago and it didn't do anything...
>>
>> I just now tried --enable-debug-pack, and as I was thinking, it had no
> effect.
>>
> What do you mean with "no effect"? Don't reduce size? The compiler/linker
> options I've mentioned are about removing identical or unreferenced 
> COMDATS,
> and they do that. BTW how do you check it? I would like you to be more
> precise at this point, please. Did you use link /map or disasm?

No effect meaning it didn't do anything at all.  So no, they didn't remove 
the, what, 220 KB+ worth of code...  File size identical, so I assume all 
contents as well (except image headers, etc.).

I've always been checking file size (for quick answer) and disassembly...

>> I don't need to solve anything on the other project since I didn't use
> static there.
>> :-P
>>
>> > Hm, probably these options could be revisited, as since 2013 there's
>> > also /Gw and /Zc:inline switches which is not implied by /Zi. But have
>> > to do more checks, for now the release build options are good enough.
>> >
>> >> Again, I'll try to compile PHP with those static's removed and report
>> >> the
>> > effect
>> >> later.
>> >>
>> > Yes, thanks for your effort. I actually didn't check what gcc does for
>> > such cases, so curious. But "static" in "static inline" forces every
>> > translation unit to have even the same function to have different
>> > address, thus eliminating the "one definition" rule for inline. We
>> > anyway need "static inline" best compatibility, the compilers handle
>> > the rest :)
>>
>> First, the report: Removing all the static's with zend_always_inline 
>> works
> fine
>> (since the __forceinline seems to "imply" static, no duplicate symbols).
> It makes
>> php7.dll 91 KB smaller (NTS --disable-all).
>>
>> But then when I tried the /Zc:inline option (really sounds like C++ on
> MSDN) the
>> other day, I was pleasantly surprised!  "You da man!" :-)
>>
>> That saved over 220 KB, without removing static's.  I verified that the
> standalone
>> functions (from static's) were gone, but obviously it also removed a lot
> more.
>> Thank you!
>>
>> Hopefully that's a switch that can be taken advantage of?
>>
> /Zc:inline is documented as C++11 feature. Still it is about enforcing the
> definition within the same translation unit, so basically kind of 
> synonymous
> to the cl/link options we have. It doesn't enforce C++11, just one that
> rule. Whether it'd break some C++ extensions - well, should check.

It sounded to me (haven't checked docs again) like it's something that sets 
whether old, MS-specific behavior is disallowed...?  e.g. nothing to break 
if it didn't [already] break with other compilers.

> But about the "static inline", it is really something that should be kept
> everywhere. It is the most convenient option for the compiler/linker
> compatibility. A global function is allowed to be defined only once. Since
> those functions are in defined in the headers, chances are to see the
> duplicated symbol errors which will prevent compilation. VC should 
> actually
> should do same.

I'm no longer asking about removing the "static" part anywhere, like 
original message, since extra junk can be removed with /Zc:inline. :-)

I get how the function definition stuff works, but VC doesn't seem to!  As 
I've already said, by default (e.g. without /Zc:inline):

*) __forceinline already seems to imply "static" by itself.  Everything 
works as expected, all compilation works fine, and there are never the 
would-be-expected duplicate symbol errors.

*) adding "static" to __forceinline, again, by default, seems to make it 
"super static" and create separate standalone versions even though they are 
never referenced.

Seems there's a bug there somewhere...

> The option using "extern inline" and splitting declaration
> and definition are unusable, because those functions have to be usable in
> external modules.
>
> In general, if testing goes good, we could add these options
> (__declspec(noinline), /Zc:inline and maybe /Gw) to master to release 
> builds
> for further observation. But it should be really good tested. We'll check 
> it
> in our labs as well. I'll be able to come to this topic either at the end 
> of
> this or early next year.

MSVC really has problems if the options mess anything up ;-), so I'm making 
the changes as I find them, hehe.

> Regards
>
> Anatol

- Matt