Merge llvm 401

[juj]

No description provided.

[juj]

All PR branches are now updated:

• merge_llvm_401 emscripten#5400
• Merge llvm 401 #191
• Merge clang 401 emscripten-fastcomp-clang#18

and all tests should pass. Ran all core suite variants and other suite on both Linux and Windows, and I don't see any failures. @kripken: these should now be all ready to merge - tag both before and after?

[kripken]

I'll start to review now, great.

[juj]

In particular check out the three commits shown above in this GitHub view, those are my changes, the first one is a "stock" merge with only merge conflicts resolved, but no other changes.

[kripken]

isn't this another case of the off-by-one issue that you just fixed in ExpandByVal?

[juj]

That is a good question. I don't actually understand where that off by one issue comes from, and what exactly changed so that I had to do that change in ExpandByVal. I did that change in ExpandByVal because I saw in test code that the expansion by value was getting applied to parameters "off by one" in the generated JS code. It may be the case that this + 1 needs to be removed, or then not. In the current branch all tests pass, so I don't want to just remove the + 1 here without understanding.

[kripken]

This gives me low confidence in the +1 removal change. I think we need to understand this better.

How about asking for help from Rust people, which they offered in the other PR? Many of them are LLVM experts. Perhaps they can give a quick and clear response on changes to the Attribute API changes in 4.0.1.

[juj]

Agreed, I am reading the changes, but I don't know who would be a good person to review this.

[kripken]

Can ask where the Rust people offered to help, in the 5.0 PR I think it was.

[juj]

Ok, commented in #199 (comment)

[juj]

Upon further inspection, it looks like the answer to this is yes.

[kripken]

Also here?

[juj]

Yes.

[juj]

Also ran the browser suite on Windows, which passes.

[juj]

Now I understand where the discrepancy comes from. The AttributeList object still uses 1-based indexing in the sense that at index 0 is the return value, and at index 1...N are the parameters to the function; but what seems to be the breaking change here is that as AttributeSet got refactored to AttributeList, the input parameter to the function AttributeList::getParamAttributes(int Index) was changed; it used to be that AttributeSet::getParamAttributes(1) returned the attributes of the first function parameter, but now what happens is that one needs to call AttributeList::getParamAttributes(0) to get the attributes of the first parameter. For other API entry points the essential 1-based indexing still exists, e.g. AttributeSet::getAttributes(i) is still the same as AttributeList::getAttributes(i), so that function (or others) did not get shifted.

Rerunning all tests after the above changes, and I'll push an updated version shortly.

[juj]

Updated with the corrected + 1 adjustments in above commit. All tests pass in core and other suites, so this is good for a second look now?

[kripken]

Thanks! Looks good. It does appear that getParamAttributes now adds FirstArgIndex (which is equal to one).

[kripken]

this can be hasParamAttribute(i)

[juj]

check

[juj]

Ok, did a new pass on this, bringing all three branches up to date with latest, and reran default and other suites, which still pass, except for other.test_llvm_lit failing. It would be great if you have a sec to look at this @kripken . What's happening is it looks like perhaps some DCE is not executed? The failure reads

test_llvm_lit (test_other.other) ... -- Testing: 29 tests, 29 threads --
******************** TEST 'LLVM :: CodeGen/JS/allocamanager-phis.ll' FAILED ********************
Script:
--
/home/clb/emsdk/clang/fastcomp/build_incoming_64/./bin/llc < /home/clb/emsdk/clang/fastcomp/src/test/CodeGen/JS/allocamanager-phis.ll | /home/clb/emsdk/clang/fastcomp/build_incoming_64/./bin/FileCheck /home/clb/emsdk/clang/fastcomp/src/test/CodeGen/JS/allocamanager-phis.ll
--
Exit Code: 1

Command Output (stderr):
--
/home/clb/emsdk/clang/fastcomp/src/test/CodeGen/JS/allocamanager-phis.ll:9:10: error: expected string not found in input
; CHECK: $vararg_buffer3 = sp;
         ^
<stdin>:2:1: note: scanning from here
// EMSCRIPTEN_START_FUNCTIONS
^
<stdin>:8:2: note: possible intended match here
 $vararg_buffer3 = sp + 16|0;
 ^

--

********************
******************** TEST 'LLVM :: CodeGen/JS/allocamanager.ll' FAILED ********************
Script:
--
/home/clb/emsdk/clang/fastcomp/build_incoming_64/./bin/llc < /home/clb/emsdk/clang/fastcomp/src/test/CodeGen/JS/allocamanager.ll | /home/clb/emsdk/clang/fastcomp/build_incoming_64/./bin/FileCheck /home/clb/emsdk/clang/fastcomp/src/test/CodeGen/JS/allocamanager.ll
--
Exit Code: 1

Command Output (stderr):
--
/home/clb/emsdk/clang/fastcomp/src/test/CodeGen/JS/allocamanager.ll:20:10: error: expected string not found in input
; CHECK: STACKTOP = STACKTOP + 128|0;
         ^
<stdin>:4:2: note: scanning from here
 $argc = $argc|0;
 ^
<stdin>:8:2: note: possible intended match here
 STACKTOP = STACKTOP + 272|0;
 ^

--

********************
********************
Failing Tests (2):
    LLVM :: CodeGen/JS/allocamanager-phis.ll
    LLVM :: CodeGen/JS/allocamanager.ll

  Expected Passes    : 27
  Unexpected Failures: 2
FAIL

======================================================================
FAIL: test_llvm_lit (test_other.other)
----------------------------------------------------------------------

I can make these two tests pass with

diff --git a/test/CodeGen/JS/allocamanager-phis.ll b/test/CodeGen/JS/allocamanager-phis.ll
index c04a212..71d5737 100644
--- a/test/CodeGen/JS/allocamanager-phis.ll
+++ b/test/CodeGen/JS/allocamanager-phis.ll
@@ -6,9 +6,9 @@
 ; an overlapping lifetime with l_766.i which is only visible if we can
 ; see through phis.

-; CHECK: $vararg_buffer3 = sp;
-; CHECK: $l_1565$i = sp + 16|0;
-; CHECK: $l_766$i = sp + 12|0;
+; CHECK: $vararg_buffer3 = sp + 16|0;
+; CHECK: $l_1565$i = sp + 36|0;
+; CHECK: $l_767$i = sp + 32|0;

 target datalayout = "e-p:32:32-i64:64-v128:32:128-n32-S128"
 target triple = "asmjs-unknown-emscripten"
diff --git a/test/CodeGen/JS/allocamanager.ll b/test/CodeGen/JS/allocamanager.ll
index 19f1ca7..d8346ab 100644
--- a/test/CodeGen/JS/allocamanager.ll
+++ b/test/CodeGen/JS/allocamanager.ll
@@ -16,11 +16,9 @@ target triple = "asmjs-unknown-emscripten"
 @.str3 = private unnamed_addr constant [43 x i8] c"another message from the program: \22%s\22...\0A\00", align 1

 ; CHECK: function _foo($argc,$argv) {
-; CHECK-NOT: cupcake
-; CHECK: STACKTOP = STACKTOP + 128|0;
+; CHECK: STACKTOP = STACKTOP + 272|0;
 ; CHECK-NEXT: vararg_buffer0 =
-; CHECK-NEXT: $muffin =
-; CHECK-NOT: cupcake
+; CHECK: $muffin =
 ; CHECK: }

 ; Function Attrs: nounwind
@@ -73,11 +71,8 @@ entry:
 }

 ; CHECK: function _bar($argc,$argv) {
-; CHECK-NOT: cupcake
-; CHECK: STACKTOP = STACKTOP + 128|0;
+; CHECK: STACKTOP = STACKTOP + 272|0;
 ; CHECK-NEXT: vararg_buffer0 =
-; CHECK-NEXT: $muffin =
-; CHECK-NOT: cupcake
 ; CHECK: }

 ; Function Attrs: nounwind

but that doesn't obviously look good. For the first test allocamanager-phis.ll I'm not sure if it's natural that there could be one more variable on the stack, bumping each stack pointer by 16 bytes. But for the second test it looks like the point of the test is to verify that cupcake gets optimized away, although it doesn't. Does this look like something sensible?

Apart from this failure, all other tests are passing ok.

[kripken]

I'm not that familiar with the AllocaManager code. @sunfishcode, you wrote it, any thoughts on what @juj is seeing above?

[juj]

The above commit fixes the AllocaManager test failures. Tested that the core and other suites now pass, so this is good to land based on everything I'm seeing.

@kripken : have we ever updated LLVM without also updating Emscripten minor version? I.e. should we do a 1.38.0 out of this, or a 1.37.30?

[juj]

Also need to update the llvm_lifetime_* names in CallHandlers.h

In that file, I see I went with having both variants:

DEF_CALL_HANDLER(llvm_lifetime_start, {
  return "";
})

DEF_CALL_HANDLER(llvm_lifetime_end, {
  return "";
})

DEF_CALL_HANDLER(llvm_lifetime_start_p0i8, {
  return "";
})

DEF_CALL_HANDLER(llvm_lifetime_end_p0i8, {
  return "";
})

DEF_CALL_HANDLER(llvm_invariant_start, {
  return "";
})

DEF_CALL_HANDLER(llvm_invariant_end, {
  return "";
})

DEF_CALL_HANDLER(llvm_invariant_start_p0i8, {
  return "";
})

DEF_CALL_HANDLER(llvm_invariant_end_p0i8, {
  return "";
})

this allows building code coming from old .bc files, I think.

[kripken]

I would also check sanity and browser.

What we've done in the past is tag a minor version before and after. So 1.37.30 and 1.37.31. (To tag 1.38.0 we should merge to master, which we should be more careful about, but we should eventually do that too.)

[juj]

Checked through browser and sanity. Browser runs fine without issues, but sanity is a bit tricky suite, because it expects an environment that emsdk-based installations are not compatible with, e.g. to run sanity, EMCC_CONFIG can't be defined, and a prebuilt Binaryen or asm.js optimizer can't be present. I could not find any failures in sanity that looked like to be caused by this PR though, so would be confident to merge this now. Do you want to do further tests before so?

[kripken]

That's ok, you can merge this. I'll run the sanity tests later, almost certainly any issue there is unrelated to LLVM changes.

[juj]

Perfect, tagging and merging and tagging now..