After llvm r195496, code compiled with -Os get substantially larger

[dim]

Summary

In FreeBSD, we have a particular piece of code, a boot loader, which must always fit in 7 kiB, so it is compiled with -Os, and various other parameters that minimize its size.

When I recently started working on importing clang 3.5.0 into the FreeBSD base system, I noticed that the code grew substantially when compiled with 3.5.0. Previously, with clang 3.4.1, it was 31 bytes below the limit of 7168 bytes, but with 3.5.0 it went 145 bytes over the limit. This is an increase of 176 bytes!

I bisected llvm trunk, and found that r195496 introduced this regression. The problem is that X86TargetLowering::EmitCmp() now expands comparisons to 32-bit operations, even with -Os. This is because it only checks for one possible size optimization attribute:

DAG.getMachineFunction().getFunction()->getAttributes().hasAttribute(AttributeSet::FunctionIndex, Attribute::MinSize)

MinSize is only in effect for -Oz. It should also check for Attribute::OptimizeForSize, which is in effect for -Os.

This diff adds the additional check, and fixes the original problem for me with the FreeBSD boot loader, which is compiled to its previous size again.

[t.p.northover]

Hi Dimitry,

In FreeBSD, we have a particular piece of code, a boot loader, which must always fit in 7 kiB, so it is compiled with -Os, and various other parameters that minimize its size.

That kind of hard size requirement sounds like what -Oz was designed for.

Lots of people use -Os as the default optimisation level, and it's
supposed to be more balanced. Given the large performance benefits and
small size penalties mentioned in the original thread, I'm not so sure
about switching this one.

Cheers.

Tim.

[dim]

FreeBSD boot2 loader size overview

See https://svnweb.freebsd.org/base/head/sys/boot/i386/boot2/

Minimized testcase is here: http://www.andric.com/freebsd/clang/boot2-minimal.c

This is being compiled with the following flags:

-cc1 -triple i386-unknown-freebsd11.0 -emit-obj -mrelocation-model static -mregparm 3 -mrtd -target-cpu i386 -Os -w -ffreestanding -mstack-alignment=8 -mllvm -inline-threshold=3 -mllvm -enable-load-pre=false -mllvm -simplifycfg-dup-ret boot2-minimal.c

The "rev" column is the llvm/clang trunk revision tested.
The "text" column is the resulting .text size of boot2.o compiled with that revision.
The "avail" column is the remaining available byte count in the final boot2 binary.
The "link" column directly links to the llvm/clang changeset.

rev	text	avail	link
r195495	5219	31	<first trunk revision tested>
r195496	5397	-145	http://llvm.org/viewvc/llvm-project?view=revision&revision=195496
r200502	5401	-149	http://llvm.org/viewvc/llvm-project?view=revision&revision=200502
r202557	5391	-141	http://llvm.org/viewvc/llvm-project?view=revision&revision=202557
r206130	5386	-133	http://llvm.org/viewvc/llvm-project?view=revision&revision=206130
r206766	5401	-149	http://llvm.org/viewvc/llvm-project?view=revision&revision=206766
r207195	5407	-157	http://llvm.org/viewvc/llvm-project?view=revision&revision=207195
r209755	5493	-241	http://llvm.org/viewvc/llvm-project?view=revision&revision=209755
r215135	5496	-245	http://llvm.org/viewvc/llvm-project?view=revision&revision=215135
r216862	5506	-253	http://llvm.org/viewvc/llvm-project?view=revision&revision=216862
r218771	5496	-245	http://llvm.org/viewvc/llvm-project?view=revision&revision=218771
r219773	5506	-253	http://llvm.org/viewvc/llvm-project?view=revision&revision=219773
r220639	5506	-253	<last trunk revision tested>

[emaste]

For reference,

FreeBSD's system clang,
FreeBSD clang version 3.4.1 (tags/RELEASE_34/dot1-final 208032) 20140512
-Os text = 5220
-Oz text = 5068

My Clang build, updated today r222429
-Os text = 5568
-Oz text = 5246

[dim]

In D5859#4, @t.p.northover wrote:

...

That kind of hard size requirement sounds like what -Oz was designed for.

Lots of people use -Os as the default optimisation level, and it's
supposed to be more balanced. Given the large performance benefits and
small size penalties mentioned in the original thread, I'm not so sure
about switching this one.

Well, as a "user" of -Os, I merely notice these size regressions, which are quite substantial (at least relative to the total code size).

In my opinion, for most users, -Os has simply meant "optimize for size", not "balance between size and speed". For example, in the latest gcc manual, the option is described as:

Optimize for size. -Os enables all -O2 optimizations that do not typically increase code size. It also performs further optimizations designed to reduce code size.

E.g. if I'd had had the original choice, I would have chosen -Oz (which is really a new option) to mean "balance between size and speed", not retroactively change the meaning of -Os.

In any case, I've also tried building this boot loader with -Oz, and in some case this made the code bigger instead. With more recent trunk, this effect seems to have reversed, though (I tested r219624):

text	   data	    bss	    dec	    hex	filename
5496	      9	   4936	  10441	   28c9	/usr/obj/with-Os/usr/src/sys/boot/i386/boot2/boot2.o
5412	      9	   4936	  10357	   2875	/usr/obj/with-Oz/usr/src/sys/boot/i386/boot2/boot2.o

[dim]

This has become obsolete now, due to apparent improvements in trunk for size optimization. See D10069 for details.

lib/Target/X86/X86ISelLowering.cpp

if ((Op0.getValueType() == MVT::i8 || Op0.getValueType() == MVT::i16 ||
        Op0.getValueType() == MVT::i32 || Op0.getValueType() == MVT::i64)) {
     // Do the comparison at i32 if it's smaller, besides the Atom case. 
     // This avoids subregister aliasing issues. Keep the smaller reference 
     // if we're optimizing for size, however, as that'll allow better folding 
     // of memory operations.
     if (Op0.getValueType() != MVT::i32 && Op0.getValueType() != MVT::i64 &&
         !DAG.getMachineFunction().getFunction()->getAttributes().hasAttribute(
              AttributeSet::FunctionIndex, Attribute::MinSize) &&
+        !DAG.getMachineFunction().getFunction()->getAttributes().hasAttribute(
+             AttributeSet::FunctionIndex, Attribute::OptimizeForSize) &&
         !Subtarget->isAtom()) {
       unsigned ExtendOp =
           isX86CCUnsigned(X86CC) ? ISD::ZERO_EXTEND : ISD::SIGN_EXTEND;
       Op0 = DAG.getNode(ExtendOp, dl, MVT::i32, Op0);
       Op1 = DAG.getNode(ExtendOp, dl, MVT::i32, Op1);
     }
     // Use SUB instead of CMP to enable CSE between SUB and CMP.
     SDVTList VTs = DAG.getVTList(Op0.getValueType(), MVT::i32);