emscripten-core / emscripten-fastcomp

LLVM plus Emscripten's asm.js backend
Other
182 stars 111 forks source link

Fix CoroFrame generation to make 8 multiples ints #226

Closed blastrock closed 4 years ago

blastrock commented 6 years ago

Hi,

I was trying to make the coroutine-ts work in emscripten and finally managed to make it work.

Here is the program I was trying to compile:

#include <stdio.h>
#include <experimental/coroutine>
#include <iostream>

#include <cppcoro/generator.hpp>

cppcoro::generator<int> usage_example()
{
  int i = 0;
  ++i;
  printf("co %d\n", i);
  co_yield i;
  ++i;
  printf("co %d\n", i);
  co_yield i;
}

int main() {
  for (auto i : usage_example())
  {
    std::cout << "main :" << i << std::endl;
  }
  return 0;
}

I managed to compile and run this by:

About this patch, this is the first time I deal with coroutines and LLVM IR, so this may not be the correct fix.

The issue I had was "LLVM ERROR: Stores must be a multiple of 8 bits". This comes from the fact that when we declare a function that make use of co_* keywords, a "Frame" structure is generated by the compiler. This structure is used to store all local variables, stuff about how to handle suspension points, and more importantly the current suspension point.

The size of the current suspension point uses just enough bits to hold the required values. In my example, this gives the following IR:

%_Z13usage_examplev.Frame = type { void (%_Z13usage_examplev.Frame*)*, void (%_Z13usage_examplev.Frame*)*, %"class.cppcoro::detail::generator_promise",

i2, // the suspension point

%"struct.std::experimental::coroutines_v1::suspend_always"*, %"struct.std::experimental::coroutines_v1::suspend_always"*, %"struct.std::experimental::coroutines_v1::suspend_always"*, %"class.std::experimental::coroutines_v1::coroutine_handle.0", %"class.std::experimental::coroutines_v1::coroutine_handle", i8*, %"class.std::experimental::coroutines_v1::coroutine_handle", i8*, %"struct.std::experimental::coroutines_v1::suspend_always"*, %"class.std::experimental::coroutines_v1::coroutine_handle.0", %"struct.std::experimental::coroutines_v1::suspend_always"*, %"struct.std::experimental::coroutines_v1::suspend_always"*, %"struct.std::experimental::coroutines_v1::suspend_always"*, %"struct.std::experimental::coroutines_v1::suspend_always"*, %"class.std::experimental::coroutines_v1::coroutine_handle.0", %"class.std::experimental::coroutines_v1::coroutine_handle", i8*, %"struct.std::experimental::coroutines_v1::suspend_always"*, %"struct.std::experimental::coroutines_v1::suspend_always"*, %"struct.std::experimental::coroutines_v1::suspend_always"*, %"class.std::experimental::coroutines_v1::coroutine_handle.0", %"class.std::experimental::coroutines_v1::coroutine_handle", i8*, %"struct.std::experimental::coroutines_v1::suspend_always", i8*, i32, %"class.std::experimental::coroutines_v1::coroutine_handle.0", %"class.std::experimental::coroutines_v1::coroutine_handle", i32, i32, %"struct.std::experimental::coroutines_v1::suspend_always", %"class.std::experimental::coroutines_v1::coroutine_handle.0", %"class.std::experimental::coroutines_v1::coroutine_handle", %"struct.std::experimental::coroutines_v1::suspend_always", i32, %"class.std::experimental::coroutines_v1::coroutine_handle.0", %"class.std::experimental::coroutines_v1::coroutine_handle", %"struct.std::experimental::coroutines_v1::suspend_always", %"class.std::experimental::coroutines_v1::coroutine_handle.0", %"class.std::experimental::coroutines_v1::coroutine_handle" }

As you can see, the coroutine has 2 suspension points. It seems that for some reason it needs one more state (maybe the start or the end of the function).

This somehow works for native code, even though there is no such thing as 2 bits integers. On x86_64, the store instructions seem to be compiled to movb:

     c6 40 18 01            movb   $0x1,0x18(%rax)

So the backend must somehow translate integers to align sizes to 8 bits.

In the emscripten backend, there's that assertion which just aborts compilation. I tried removing the assertion and fixing the backend but it involved updating lots of functions like getPromotedType, isLegalSize and shouldConvert. That's why I fixed the size of the field in LLVM.

Is there any reason why the field is generated as i2? Is it to support some exotic architecture? Do you think this is an acceptable fix?

kripken commented 6 years ago

I don't fully understand the details here, but in general we try to avoid modifying LLVM code as much as possible (and just add our JS backend, nothing else, in this fork). One option might be to talk to upstream LLVM about this change, as maybe it makes sense in general (I'm not sure). Another is to improve i2 handling in our JS backend, which would be in one of the passes in lib/Target/JSBackend/NaCl/ that legalizes code (it can handle most LLVM IR constructs, whatever we've seen is necessary from typical C/C++ output from clang, but I guess i2s like that were not seen yet).