-
Notifications
You must be signed in to change notification settings - Fork 206
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Please test 1.0.0-beta1 #323
Comments
Note: Linux/Mac binaries are still coming, so check back in a bit if you don't see them yet. |
Test My simple sandbox code seems to work fine too but it doesn't really stress much. :-) |
Was looking through the docs to see if I could test something that might break and found a doc bug. On http://terralang.org/api.html#function the |
I think that's just saying that the argument is optional, and has a default value of If you have ideas for how to make it more clear, please create a new issue---I think this will be pretty pervasive in the docs so it won't be a quick and simple change. |
We could probably have a matrix of test conditions here and check them all. Also, I think it is nice to have dependent projects like
|
@ProfFan Ok, I copied your test matrix into the issue description up top. Would you be willing to test Opt? Also, do you know any contacts for Darkroom? I'm all in favor of having everyone test this, but I'm not going to know how to run their tests myself. |
FYI, I noticed an issue where the macOS binary that got uploaded did not have CUDA enabled (due to a bug in a condition on Travis). I've renamed that binary, and re-run the correct Travis job, so the new binary should have CUDA enabled. |
Well, I tried. Namely:
And copying the terra binary to the tests folder and running from there (neither method is in the README, which I think doesn't correspond to the zip folder structure). Only the first one sort of went through, producing a lot of:
In the end:
But quickly looking at the log, a good chunk of the fails was due to the fact that I've tried numerous standard library headers, including GCC (doesn't compile), MSVS (LLVM says that code isn't executable), and Here's the full test log. |
And oh, Darkroom tests seem to be failing because of the same issue. |
@elliottslaughter I will test Opt on macOS and Linux in a few days. |
@bananu7 I think darkroom relies on platform specific things, such as pthreads:
|
@Clyybber Would be worth a try to test it with MinGW |
I grabbed a Windows 10 machine at work that has cygwin and the visual studio tools and a lot of other dev stuff that no one uses here anymore and the best I was able to get the tests to do was Full failing list: =================
= FAILING tests
=================
ainline.t
arrayt.t
atoi.t
avxhadd.t
benchmark_fannkuchredux.t
benchmark_nbody.t
bf.t
blocking.t
blocking2-fixed.t
blocking2.t
blocking3.t
bounce.t
breaktest.t
bug2.t
bug4.t
cconv.t
clanginfo.t
class.t
class2.t
class3.t
class5.t
class6.t
clean.t
defer.t
deferbreak.t
defergoto.t
dgemm.t
dgemm2.t
dgemm3.t
diffuse.t
dynlib.t
exportdynamic.t
fakeasm.t
gemm.t
hello.t
hello2.t
includec.t
luaapi.t
malloc.t
mathlib.t
multiterra.t
new.t
objtest.t
option_e.t
output.t
printfarray.t
printfloat.t
proxystruct.t
quote7.t
quote8.t
quote9.t
quoteselect.t
recstruct.t
renaming.t
scc.t
sgemm-old.t
sgemm.t
sgemm3.t
sgemmkernel.t
sharedlib.t
speed.t
ssimple.t
stencil.t
strerror.t
string.t
symbolmangling.t
testminmax.t
teststd.t
testvector.t
varargcstring.t
=================
453 tests passed. 70 tests failed. I tested gcc and linking with some simple C/C++ programs that used some standard headers with no issue, and likewise with the VC tools and nmake'ing a simple project. Note that it is the Visual Studio compiler that is installed, not the full IDE, but the entire compiler pipeline and VS headers and all are installed. |
I can test something else out if needed as well. If needing of anything 'too' windows specific I may need help with as I'm a linux user, not a Windows user, and thus I don't know Windows quite perfectly, but I do have a bash shell and all the VS and cygwin tools available. For note, the failing tests had a LOT of |
@bananu7 @OvermindDL1 For your Windows tests, what Windows and Visual Studio versions do you have installed? Probably what's happening is we're hitting these lines: https://github.com/zdevito/terra/blob/master/src/terralib.lua#L4068-L4069 The binaries are built with Visual 2015, but those values are hard-coded to expect 2013. However, when I tried to remove those and/or reorder them with respect to the Clang paths, I started to get some failing tests on AppVeyor. But it may still be possible to make this work better than it currently is. |
Oh wow. This is seriously sketchy. Should be definitely taken from an environment variable or similar. I'm at home now with pretty much all VS versions installed, so giving it a try again. On second thoughts, why can't those headers be bundled together with Terra? Is there some licensing issue there? |
OK, so the results from my machine are in:
Seems that the include situation isn't really much better:
I have no idea why it's using the 12.0 |
I'm pretty sure that this is the problematic part. The fact that it, sort of maliciously, inserts the 14.0 path can be observed by running And indeed, disabling (temporarily changing the name of the folder) the 14.0 includes makes it work - all tests pass. 😄 |
This was the relevant pull request: #315, you can see why that part was added (or rather, rearranged) in there. I hadn't gotten the bandwidth to figure out how fix it properly. |
@bananu7 I'd love to accept a pull request for this. As you can see in the discussion of #315, I spent some time trying to figure out how to fix it, but debugging via AppVeyor builds is about the slowest debugging cycle you can possibly imagine, and I eventually gave up on it. Someone with a working Visual Studio install could probably iterate a lot faster. I see two possible options:
|
2017 I think I remembered seeing? Whatever was the latest available toolset earlier this year most likely.
Not if you use clang's headers. :-)
Honestly I'd say just do this, even Visual Studio 2017+ itself supports clang now. You'd have to use a very recent clang version though.
For note, I'm not entirely sure that the Visual Studio Tools installs registry keys, I think it just updates the PATH and maybe some other envvars? It's basically the pure-cli version of Visual Studio designed for CI builds and so forth. |
This version is not building for me on macOS High Sierra. The error is:
|
@ProfFan Where did you get your LLVM? I'm also on 10.13.6, and I've had trouble building with certain binaries (though I can't remember ever seeing that exact message), but so far no trouble whenever I build LLVM from source. |
@elliottslaughter I think it is not a problem with LLVM, as the failure is at the LuaJIT building. |
@ProfFan Could you upload the full log somewhere? Might also be good to start from a fresh clone in case there is any residue left over. |
I tested the binary release build on Windows 10, Arch Linux, and Ubuntu Under windows 10 with Visual Studio Community Edition 2017, I see the same symptom as mentioned previously with the failure to find the specified registry entry. I see the same set of failing tests with the addition of rd.t. However, all the cuda tests passed. Under Arch Linux, I initially see a failure due to being unable to load libtinfo.so.5 because Arch Linux provides libtinfo.so.6. After putting a symlink to libtinfo.so.6 on the LD_PATH for Terra, Terra runs but writes to the terminal an error message stating Testing darkroom from latest version on github produces the following error message
On Ubuntu 16.04, 523 tests pass, 0 failures. Darkroom produces the same error under Ubuntu as under ArchLinux |
@elliottslaughter Finally found the problem. I accidentally linked |
@elliottslaughter Tested the test suite & Opt with CUDA on macOS, so far no problem :) Darkroom produces the same error as @aiverson 's, probably we can ignore Darkroom for now? Also, are there any other important downstream projects? |
@jameshegarty Would you be interested in helping to test Darkroom against the new beta release of Terra? See above for errors we're seeing. ^^ |
I dno if it makes sense to test Darkroom, since I'm not really maintaining that code (I'm not even sure if it works with the last public release of terra from a few years ago). But we should try it with Rigel (https://github.com/jameshegarty/rigel). you can just do: I will try running this myself ASAP |
@memophen look at https://github.com/luapower/terra/blob/master/terralib_luapower.lua for how to set systemincludes disas() doesn't give assembler code on Windows only LLVM IR (works on Linux though) saveobj() detects binary type based on file extension IIRC so try it with 'helloterra.exe' do you have stdio.h on your hard drive? find it manually then modify terralib.lua to point to it, it doesn't have to be VS 12 |
@capr (1) The CUDA_PATH method did the trick, no complaints about stdio.h anymore. Setting
The script stops before executing:
(2) I can accept (3) Adding the (4) There is no terralib.lua in the build (terra-Windows-x86_64-2e2032e.zip). My assumption of a reference to v12.0 was based on the source I downloaded separately (terra-release-1.0.0-beta1.zip). |
This very helpful error message is caused when llvm can't find the linker (link.exe on Windows); a recent pull (#400) partially fixed this, as long as you run terra from the 'Developer Command Prompt' or 'x64 Native Tools Command Prompt' which set the right environment variables. |
After going through this thread, I am alarmed at the amount of development being done by developers who simply do not use Windows, don't know how it works, and in some cases seem to aggressively not care, so I want to make a few things clear:
|
Just a couple of notes: What you refer to as the C runtime is actually the C++ runtime (msvcp*.dll). The C runtime is msvcrt.dll and it's a Windows built-in since forever. You can safely link against that without redistribution issues but that's only for C programs. C++ programs must link msvcp statically or tell the user to download it from ms website since you're not licensed to include it yourself. About mingw not working correctly, the entire luapower collection (including luajit, terra, llvm and binaries built with terra) is built on mingw64 since the beginning and I never had a bug from that. Luapower's terra.dll btw is portable, you can check it out here: https://github.com/luapower/terra/tree/master/bin About the whole compiler detection thing, I personally wouldn't have done any of that, just add mention in the docs that you need to set the include paths of your CRT (be that VC or mingw depending on how you've built terra) if you want to use C headers from Terra. Every programmer would understand. |
There are legends about mingw being poorly behaved in other projects. If it happened to work for you, congratulations, but I will never recommend it to anyone doing windows development. I double checked and it looks like terra is linking against the C library, not the C++ one, so the executables being produced are actually portable, it is only terra itself that needs to be statically linked so we stop copying |
I'd be fine with statically linking On Linux I think the holy build box approach probably makes more sense (i.e. dynamically link an old glibc for maximum compatibility, and statically link everything else), but if people want to convince me otherwise, by all mean do so. I think we're not going to be able to get away without needing a full C compiler infrastructure at least at the point where Terra is being used to compile something. Since as @blackhole12 noted, we can compile arbitrary C code, and I consider this a feature not a bug (for exactly the reason why Dragon FFI exists when CFFI was already a thing). But if there are things we can do to improve the portability of Terra (so you don't need the same compiler/distro when using Terra to compile something as when you built it) then I'm all ears. |
If Terra ever actually gets a standard library, it would be possible to build a "Terra runtime" which then contains whatever parts of the C standard library that the Terra standard library wraps around. This would allow you to compile terra code that only depends on the Terra standard library with no external C code without needing the Windows SDK or Visual Studio. This would still require switching to the |
I forgot to add to my last comment: I'd be happy to have better MinGW support in Terra, and I think it could live alongside the existing MSVC support. But (as with all things in the project) it will have to come from someone who uses that platform on a regular basis and is willing to put in the time to make the improvements. There's a reason why the MSVC support has been advancing so rapidly recently.... |
Enough has changed recently that I'm planning to cut another beta release. Please let me know if there are any issues that should block that. (I consider documentation issues blockers for 1.0 but not for the next beta.) |
I was originally going to suggest making another release once switch statements were added, but that has apparently devolved into a debate about whether it should even exist, so that's not going to happen. I would consider trying to rebuild LLVM by statically linking the C++ runtime instead of shipping the dynamic library, but I haven't made any progress with that. |
I do want to get switch or jump tables into a release, but I think it's worth taking the time to do them right. I'm happy to do releases more frequently, assuming there is something worth releasing. So if we get switches shortly after this beta we can just do another one. |
I don't think there are any really serious blocking issues at the moment, but nobody's actually gone back to check if we've been able to improve the situation in #366, which I believe was partially caused by the include header problem that I fixed. I'm hoping that my fix makes the result more portable, but we'd have to find the people who were having trouble installing terra and ask them to try again. Of course, on the other hand, it'd be a lot easier to test if we have a beta build for them to test installation with, so maybe the best course of action is to simply make a second beta release and then use that to run more tests. |
I'm happy to test things on Windows if there are binaries available (so more frequent releases are better for me). And I believe the newly-introduced "github actions" (github's own CI/CD) does support Windows hosts, and claims to be free for public repos. |
I think you can always get the most recent build off of AppVeyor, e.g. here's yesterday's build: https://ci.appveyor.com/api/buildjobs/3q0q8a9nccb0onxv/artifacts/terra-Windows-x86_64-8c6ccd0.zip I wouldn't mind considering a more unified CI platform; we've got an issue about it at #355. The main limiter (as always) is someone's time to play around with the options and find something that works better than our current solution. |
I'd like to have improved RAII/memory management stuff in for the initial release of 1.0, but I don't think it should block making another beta release. I think that there should be syntax support for some other things, but they don't need to all get implemented at once.
|
Testing on a Google Compute Engine; clean Ubuntu 18.04 installation. Installed Cuda 10.2; the GPU is a Tesla T4 (which is compute architecture 7.5, which is the latest). The cuda tests all fail with:
I'm guessing the problem is that this GPU's compute capability is higher than the highest supported by whichever version of LLVM produced the binaries, and the proper fix is to query LLVM for the highest supported and clamp the version to that. I'll give it a shot building from source in the coming days. |
Just a side note: I recently upgraded my Mac to macOS Catalina, so I will not be able to test macOS + CUDA anymore as NV does not provide drivers for 10.14+... But anyway the user base is small so I assume that will not hurt too much... |
If you have time it would be good to check there are no new issues on Catalina (i.e. relative to #365, which is still outstanding). |
Since we have a beta2 now, maybe this should be updated? |
Yeah, this issue has become large enough to be unmanageable, I'll regenerate the issue and copy the remaining todos. |
New issue is at #439. Please move all new discussion there. |
Just wanted to follow up on this. Since the changes in #471 (comment) I'm getting equivalent performance with LLVM 13. It may be worth trying this out on Opt, if this is still something you care about. |
CCing people who have been active recently or who have contributed to the recent release: @viluon @bananu7 @OvermindDL1 @Mx7f @ProfFan @Clyybber
I've put together a beta for the upcoming 1.0 release. If you have the time, please download it and make sure it is working. I'm particularly looking for someone to test the Windows build.
If no major issues are found I'll release this as 1.0.
Test matrix for release:
Other todo items:
label([name])
andand
for multiple functions)defer
)__for
(and question about iterators) #333 (document__for
)lex:terraexpr()
andlex:terrastats()
)Optional:
The text was updated successfully, but these errors were encountered: