fastcomp: extractvalue generates unaligned loads

[waywardmonkeys]

Here's a pattern that pops up in code from embind:

 $method = $method | 0;
 $wireThis = $wireThis | 0;
 var $$01$i$i = 0, $$field = 0, $$field2 = 0, $10 = 0, $14 = 0, $15 = 0, $22 = 0, $24 = 0, $25 = 0, $28 = 0, $34 = 0, $35 = 0, $40 = 0, $42 = 0, $45 = 0, $46 = 0, $51 = 0, $8 = 0, $9 = 0, $x$01$i$i$i$i$i$i$i$i = 0, sp = 0;
 sp = STACKTOP;
 $$field = HEAPU8[$method >> 0] | HEAPU8[$method + 1 >> 0] << 8 | HEAPU8[$method + 2 >> 0] << 16 | HEAPU8[$method + 3 >> 0] << 24;
 $$field2 = HEAPU8[$method + 4 >> 0] | HEAPU8[$method + 5 >> 0] << 8 | HEAPU8[$method + 6 >> 0] << 16 | HEAPU8[$method + 7 >> 0] << 24;
 if (!($$field2 & 1)) $8 = $$field; else $8 = HEAP32[(HEAP32[$wireThis + ($$field2 >> 1) >> 2] | 0) + $$field >> 2] | 0;
 $9 = FUNCTION_TABLE_ii[$8 & 2047]($wireThis + ($$field2 >> 1) | 0) | 0;
 ...

We can see that $$field and $$field2 are unaligned reads.

That's from this C++:

        template<typename MemberPointer,
                 typename ReturnType,
                 typename ThisType,
                 typename... Args>
        struct MethodInvoker {
            static typename internal::BindingType<ReturnType>::WireType invoke(
                const MemberPointer& method,
                typename internal::BindingType<ThisType>::WireType wireThis,
                typename internal::BindingType<Args>::WireType... args
            ) {
                return internal::BindingType<ReturnType>::toWireType(
                    (internal::BindingType<ThisType>::fromWireType(wireThis)->*method)(
                        internal::BindingType<Args>::fromWireType(args)...
                    )
                );
            }
        };

And this is the LLVM IR involved:

  %1 = load { i32, i32 }* %method, align 4, !tbaa !22
  %2 = extractvalue { i32, i32 } %1, 1
  %3 = ashr i32 %2, 1
  %4 = bitcast %"class.northstar::PhysicsSkeletonComponent"* %wireThis to i8*
  %5 = getelementptr inbounds i8* %4, i32 %3
  %6 = bitcast i8* %5 to %"class.northstar::PhysicsSkeletonComponent"*
  %7 = extractvalue { i32, i32 } %1, 0
  %8 = and i32 %2, 1
  %9 = icmp eq i32 %8, 0
  br i1 %9, label %16, label %10

Now, the old (non-fastcomp) compiler pretended everything in an extractvalue was aligned:

              case 'extractvalue': { // XXX we assume 32-bit alignment in extractvalue/insertvalue,
                                     // but in theory they can run on packed structs too (see use getStructuralTypePartBits)

However, in fastcomp, there is the ExpandStructRegs pass in lib/Transforms/NaCl/ExpandStructRegs.cpp and it throws away the alignment in ProcessLoadOrStoreAttrs:

  // Make a pessimistic assumption about alignment.  Preserving
  // alignment information here is tricky and is not really desirable
  // for PNaCl because mistakes here could lead to non-portable
  // behaviour.
  Dest->setAlignment(1);

All of that said ... if I try to preserve the alignment by modifying the above to be:

  Dest->setAlignment(Src->getAlignment());

Then it ends up generating this JS for the $$field and $$field2 loads:

 $$field = HEAP32[$method >> 2] | 0;
 $$field2 = HEAP32[$method + 4 >> 2] | 0;

[waywardmonkeys]

cc: @chadaustin

[waywardmonkeys]

cc: @chadaustin

[waywardmonkeys]

The code that emits this stuff in the first place for a member function pointer load is here:

https://github.com/kripken/emscripten-fastcomp-clang/blob/562a45b54cb36f20c87b20fe035a66130d740bf7/lib/CodeGen/ItaniumCXXABI.cpp#L265

[waywardmonkeys]

The code that emits this stuff in the first place for a member function pointer load is here:

https://github.com/kripken/emscripten-fastcomp-clang/blob/562a45b54cb36f20c87b20fe035a66130d740bf7/lib/CodeGen/ItaniumCXXABI.cpp#L265

[waywardmonkeys]

In the IMVU codebase, this change to use aligned loads appears to result in an 80k size reduction in the minified build and nearly 200k in the -g3 debug build.

[waywardmonkeys]

In the IMVU codebase, this change to use aligned loads appears to result in an 80k size reduction in the minified build and nearly 200k in the -g3 debug build.

[kripken]

I don't think we can just assume full alignment, but taking into account the struct's alignment plus the field's alignment inside it should give the right result. I don't remember how to do that in llvm, but there is a method that does this, I am fairly sure. Likely @sunfishcode would know off the top of his head.

[kripken]

I don't think we can just assume full alignment, but taking into account the struct's alignment plus the field's alignment inside it should give the right result. I don't remember how to do that in llvm, but there is a method that does this, I am fairly sure. Likely @sunfishcode would know off the top of his head.

[chadaustin]

Isn't there an option to assume full alignment? The old compiler had an option like that, iirc. It's undefined behavior in C to rely on unaligned reads so, at least for our use case, I would prefer ALL memory operations have full alignment.

[chadaustin]

Isn't there an option to assume full alignment? The old compiler had an option like that, iirc. It's undefined behavior in C to rely on unaligned reads so, at least for our use case, I would prefer ALL memory operations have full alignment.

[kripken]

There isn't currently an option for force full alignment in fastcomp, sounds ok to me to add one.

[kripken]

There isn't currently an option for force full alignment in fastcomp, sounds ok to me to add one.

[sunfishcode]

Offfhand, I don't think Emscripten on asmjs-unknown-emscripten ever needs to discard alignment information, in contrast to the needs of PNaCl which does. I think we can do "full alignment" by default.

[sunfishcode]

Offfhand, I don't think Emscripten on asmjs-unknown-emscripten ever needs to discard alignment information, in contrast to the needs of PNaCl which does. I think we can do "full alignment" by default.

[kripken]

Yes, we don't need to discard alignment information, as we can depend on JS portability, unlike PNaCl. We should preserve alignment information here, as mentioned above.

However, I don't think we can do full alignment by default. While undefined behavior, it would break many real-world apps, e.g. Cube 2/BananaBread. But again, adding an option for full alignment sounds fine.

[kripken]

Yes, we don't need to discard alignment information, as we can depend on JS portability, unlike PNaCl. We should preserve alignment information here, as mentioned above.

However, I don't think we can do full alignment by default. While undefined behavior, it would break many real-world apps, e.g. Cube 2/BananaBread. But again, adding an option for full alignment sounds fine.

[waywardmonkeys]

Can we talk about this again?

[waywardmonkeys]

Can we talk about this again?

[kripken]

Sure. Is there new data or other info? Re-reading back here, I would be interested to see whether Cube 2 does in fact break with full alignment. I'm 99% sure it would, but maybe I am missing something. That could easily change my position here.

[kripken]

Sure. Is there new data or other info? Re-reading back here, I would be interested to see whether Cube 2 does in fact break with full alignment. I'm 99% sure it would, but maybe I am missing something. That could easily change my position here.

[kripken]

The pnacl pass code looks fixed on the merge-pnacl-mar-13-2015 branches, so we might see this fixed next week or so if those merge to incoming.

[kripken]

The pnacl pass code looks fixed on the merge-pnacl-mar-13-2015 branches, so we might see this fixed next week or so if those merge to incoming.

[waywardmonkeys]

Oddly, while the LLVM IR still has some extractvalues without alignment, it seems like the generated JS is much better:

function __ZN10emscripten8internal13MethodInvokerIMN9northstar6CanvasEFvRKN4gmtl3VecIiLj2EEEEvPS3_JS8_EE6invokeERKSA_SB_PS6_(i3, i1, i2) {
 i3 = i3 | 0;
 i1 = i1 | 0;
 i2 = i2 | 0;
 var i4 = 0;
 i4 = HEAP32[i3 >> 2] | 0;
 i3 = HEAP32[i3 + 4 >> 2] | 0;
 if (i3 & 1) i4 = HEAP32[(HEAP32[i1 + (i3 >> 1) >> 2] | 0) + i4 >> 2] | 0;
 FUNCTION_TABLE_vii[i4 & 1023](i1 + (i3 >> 1) | 0, i2);
 return;
}

Closing.

[waywardmonkeys]

Oddly, while the LLVM IR still has some extractvalues without alignment, it seems like the generated JS is much better:

function __ZN10emscripten8internal13MethodInvokerIMN9northstar6CanvasEFvRKN4gmtl3VecIiLj2EEEEvPS3_JS8_EE6invokeERKSA_SB_PS6_(i3, i1, i2) {
 i3 = i3 | 0;
 i1 = i1 | 0;
 i2 = i2 | 0;
 var i4 = 0;
 i4 = HEAP32[i3 >> 2] | 0;
 i3 = HEAP32[i3 + 4 >> 2] | 0;
 if (i3 & 1) i4 = HEAP32[(HEAP32[i1 + (i3 >> 1) >> 2] | 0) + i4 >> 2] | 0;
 FUNCTION_TABLE_vii[i4 & 1023](i1 + (i3 >> 1) | 0, i2);
 return;
}

Closing.