Update to LLVM 4.0

[dylanmckay]

This is accompanied with emscripten-core/emscripten-fastcomp-clang#13

[dylanmckay]

There are two failing JS CodeGen tests

test/CodeGen/JS/invariant-intrinsics.ll

This is a test to ensure that we don't emit calls/declarations to LLVM intrinsics (such as llvm.invariant.start).

The test is failing because we actually are emitting declarations and calls to these intrinsics.

Here is the output of llc on the test:

// EMSCRIPTEN_START_FUNCTIONS
function _foo() {
 var $0 = 0, $p = 0, label = 0, sp = 0;
 sp = STACKTOP;
 STACKTOP = STACKTOP + 16|0;
 $p = sp;
 $0 = (_llvm_invariant_start_p0i8(1,0,($p|0))|0);
 _bar(($p|0));
 _llvm_invariant_end_p0i8(($0|0),1,0,($p|0));
 STACKTOP = sp;return;
}
function runPostSets() {
}
// EMSCRIPTEN_END_FUNCTIONS

/* memory initializer */ allocate([], "i8", ALLOC_NONE, Runtime.GLOBAL_BASE);


// EMSCRIPTEN_METADATA
{
"staticBump": 8,
"declares": ["bar", "llvm_invariant_start_p0i8", "llvm_invariant_end_p0i8"],"redirects": {},"externs": [],"implementedFunctions": ["_foo"],"tables": {},"initializers": [],"exports": [],"aliases": {},"cantValidate": "","simd": 0,"simdUint8x16": 0,"simdInt8x16": 0,"simdUint16x8": 0,"simdInt16x8": 0,"simdUint32x4": 0,"simdInt32x4": 0,"simdFloat32x4": 0,"simdFloat64x2": 0,"simdBool8x16": 0,"simdBool16x8": 0,"simdBool32x4": 0,"simdBool64x2": 0,"maxGlobalAlign": 4,"namedGlobals": {},"asmConsts": {}
}

When I step through with a debugger, there is a check which no longer does what it should (in JSBackend.cpp::printModuleBody).

When generating the declares: [ .. ]array, we step through all declarations, check if it is an intrinsic, and possibly skips it.

This uses I->isIntrinsic(), which in turn just checks if the intrinsic name starts with "llvm.". When I inspect I->getName, it is actually llvm_. I am unsure why or where the dots were converted into underscores, but it causes LLVM to not pick up the fact these are intrinsics.

It seems like this may have been broken in D22949 it was not.

CodeGen/JS/getelementptr.ll

Not looked into yet.

[kripken]

What might be going on in that test is LLVM may have renamed the intrinsic a little. In the test we used to have llvm.invariant.start, but I guess in LLVM 4.0 it was renamed to add a suffix, giving llvm.invariant.start.p0i8. We handle such things in lib/Target/JSBackend/CallHandlers.h, look for llvm_invariant_start (note the underscores - yeah, we do that renaming, so it is a valid JS name, and we do it before we get there). So maybe just updating the name in that function will fix things.

[kripken]

(All we do in CallHandlers for that method is return an empty string, which is the right thing for that intrinsic; otherwise, if no handler is found in that file, we emit a call to what we assume is something imported into asm/wasm, which is why it looks as it does in your output sample.)

[dylanmckay]

That worked! I've fixed that test, looking into the other one now.

[dylanmckay]

I believe the second test is probably broken by D26594, where the behaviour of GetElementPtrTypeIterator was changed (including the dereference operator being removed!).

Here is the diff of emscripten master vs llvm-4.0 on JSBackend.cpp; take a look specifically at JSWriter::generateExpression

diff --git a/lib/Target/JSBackend/JSBackend.cpp b/lib/Target/JSBackend/JSBackend.cpp
index d959102..6ea52c7 100644
--- a/lib/Target/JSBackend/JSBackend.cpp
+++ b/lib/Target/JSBackend/JSBackend.cpp
@@ -270,7 +270,7 @@ namespace {
         OptLevel(OptLevel), StackBumped(false), GlobalBasePadding(0), MaxGlobalAlign(0),
         CurrInstruction(nullptr) {}
 
-    const char *getPassName() const override { return "JavaScript backend"; }
+    StringRef getPassName() const override { return "JavaScript backend"; }
 
     bool runOnModule(Module &M) override;
 
@@ -2658,14 +2658,14 @@ void JSWriter::generateExpression(const User *I, raw_string_ostream& Code) {
     for (GetElementPtrInst::const_op_iterator E = GEP->op_end();
        I != E; ++I) {
       const Value *Index = *I;
-      if (StructType *STy = dyn_cast<StructType>(*GTI++)) {
+      if (StructType *STy = GTI.getStructTypeOrNull()) {
         // For a struct, add the member offset.
         unsigned FieldNo = cast<ConstantInt>(Index)->getZExtValue();
         uint32_t Offset = DL->getStructLayout(STy)->getElementOffset(FieldNo);
         ConstantOffset = (uint32_t)ConstantOffset + Offset;
       } else {
         // For an array, add the element offset, explicitly scaled.
-        uint32_t ElementSize = DL->getTypeAllocSize(*GTI);
+        uint32_t ElementSize = DL->getTypeAllocSize(GTI.getIndexedType());
         if (const ConstantInt *CI = dyn_cast<ConstantInt>(Index)) {
           // The index is constant. Add it to the accumulating offset.
           ConstantOffset = (uint32_t)ConstantOffset + (uint32_t)CI->getSExtValue() * ElementSize;
@@ -4142,7 +4142,7 @@ Pass *createCheckTriplePass() {
 bool JSTargetMachine::addPassesToEmitFile(
       PassManagerBase &PM, raw_pwrite_stream &Out, CodeGenFileType FileType,
       bool DisableVerify, AnalysisID StartBefore,
-      AnalysisID StartAfter, AnalysisID StopAfter,
+      AnalysisID StartAfter, AnalysisID StopBefore, AnalysisID StopAfter,
       MachineFunctionInitializer *MFInitializer) {
   assert(FileType == TargetMachine::CGFT_AssemblyFile);

The previous code was iterating through all type parameters, and then incrementing the iterator again inside the first condition. This is weird, and I can't really find the reason for it.

When I change the code to (GTI++).getStructTypeOrNull(), it causes the else block to hit an assertion error. Because we increment GTI in the predicate for the if statement, when we fall through to the else block, GTI == GEP.op_end() and *GEP == nullptr, causing the dynamic cast to fail.

When you look at the test itself, you'll see this:

; CHECK: function _getelementptr([[VAL_P:\$[a-z_]+]]) {
; CHECK:  [[GEP:\$[a-z_]+]] = ((([[GEPINT:\$[a-z_]+]])) + 588|0);
define i32* @getelementptr([10 x [12 x i32] ]* %p) {
  %t = getelementptr [10 x [12 x i32]], [10 x [12 x i32]]* %p, i32 1, i32 2, i32 3
  ret i32* %t
}

Comparing it to the output of llc, you'll notice that it is almost correct, but instead of + 558, it is + 2880. I've been playing around with the if statement but I haven't been able to get it to pass, as I haven't actually used emscripten before.

[kripken]

Hmm, I don't understand that LLVM commit. I'd need to read more to get the context, I guess - seems like it's part of the big years-long GEP or pointer type changes, somehow? But regarding

The previous code was iterating through all type parameters, and then incrementing the iterator again inside the first condition. This is weird, and I can't really find the reason for it.

It always increments GTI, since the ++ happens in the loop's first condition, which is always executed. It then enters either the first or the second condition. So each loop iteration increments once.

You probably just need to increment GTI in the first condition. Perhaps less confusing would be to increment it before, saving the old value and using that.

[dylanmckay]

I debugged it a bit further and got it working by moving the incrementing of GTI into the loop definition.

That test is now passing, I'm running the LLVM test suite locally, then I'll look at running the emscripten tests.

[kripken]

I created next-merge branches that mirror yours and started to do some testing. First thing I noticed is clang gave an error on startup, which I tracked down to the wasm backend now having emscripten-cxx options which collide with ours (we did it first! ;) ). So I renamed those in

1aaa617

And after that things look promising! 30 random tests pass. I'll run the full test suite next.

[kripken]

Running the test suite overnight found a bunch of weird errors, that I tracked down to 63e73b3 . Not 100% sure about it, but seems to fix things.

[kripken]

Ok, after that fix, I found a few small test changes that were necessary, pushed to the next-merge branch in the emscripten repo. Nothing major, just llvm's opts work slightly differently here and there.

[kripken]

Ok, after a few more fixes across the repos, I think this merge is almost ready. I opened #166 and parallel PRs with your stuff + mine. As far as I can tell all tests pass there.

Thanks @dylanmckay for all the work here! Much appreciated :)

[brson]

Thanks @dylanmckay and @kripken for working on this port!

[dylanmckay]

Superseded by #166