Skip to content

Conversation

@aykevl
Copy link
Member

@aykevl aykevl commented Dec 7, 2019

This PR comes in two parts:

  1. A refactor that makes the wasm ABI testable.
  2. A small commit that moves temporary allocas to the entry block, which should fix some cases of LLVM ERROR: Coroutines cannot handle non static allocas yet #234 and Possibly spurious error: LLVM ERROR: Coroutines cannot handle non static allocas yet #421.

There are possible future improvements (such as the readonly attribute) but I left them alone for now as that's not needed to fix these bugs.

@aykevl
Copy link
Member Author

aykevl commented Dec 7, 2019

I modified the first commit a bit (https://github.com/tinygo-org/tinygo/compare/aa05d54db780c99ae34f0bd6a5f01a788345b79b..1f2982b22f3fd6d004668504c360cef2ad23f258) because there was a bug in the transform (surfacing only with the added tests) that caused CI to fail.

@aykevl aykevl force-pushed the wasm-abi-alloca branch 2 times, most recently from b7f5dca to 87a0212 Compare December 8, 2019 00:15
@aykevl
Copy link
Member Author

aykevl commented Dec 8, 2019

I saw some more non static allocas. I think they are created by the coroutine lowering pass (@jaddr2line).

This is the IR I found:

define internal fastcc { %"github.com/aykevl/go-smartwatch.Watch"*, %runtime._interface } @"github.com/aykevl/go-smartwatch.Open"(i8* nocapture readnone %context, i8* %parentHandle) unnamed_addr #0 section ".text.github.com/aykevl/go-smartwatch.Open" !dbg !2376 {
entry:
  %complit17.i = alloca %"tinygo.org/x/drivers/st7789.Device", align 8, !dbg !2378
  %complit10.i = alloca %machine.PinConfig, align 8, !dbg !2378
  %complit3.i = alloca %machine.PinConfig, align 8, !dbg !2378
  %complit.i = alloca %machine.PinConfig, align 8, !dbg !2378
  %bus.i = alloca %machine.SPI, align 8, !dbg !2378
  %config.i = alloca i64, align 8, !dbg !2392
  %spi.i = alloca %machine.SPI, align 8, !dbg !2392
  %task.state = alloca %runtime.taskState, align 8, !dbg !2408
  %task.token = call token @llvm.coro.id(i32 0, i8* nonnull null, i8* bitcast ({ %"github.com/aykevl/go-smartwatch.Watch"*, %runtime._interface } (i8*, i8*)* @"github.com/aykevl/go-smartwatch.Open" to i8*), i8* null), !dbg !2408
  %task.size = tail call i32 @llvm.coro.size.i32(), !dbg !2408
  %task.data = tail call fastcc i8* @runtime.alloc(i32 %task.size), !dbg !2408
  %task.handle = call noalias nonnull i8* @llvm.coro.begin(token %task.token, i8* nonnull %task.data), !dbg !2408
  %FramePtr = bitcast i8* %task.handle to %"github.com/aykevl/go-smartwatch.Open.Frame"*, !dbg !2408
  %parentHandle.spill.addr = getelementptr inbounds %"github.com/aykevl/go-smartwatch.Open.Frame", %"github.com/aykevl/go-smartwatch.Open.Frame"* %FramePtr, i32 0, i32 4, !dbg !2408
  store i8* %parentHandle, i8** %parentHandle.spill.addr, !dbg !2408
  tail call void @runtime.trackPointer(i8* nonnull %task.data, i8* undef, i8* null), !dbg !2408
  call void @llvm.dbg.value(metadata i8* %parentHandle, metadata !2409, metadata !DIExpression()), !dbg !2412
  %0 = icmp eq i8* %parentHandle, null, !dbg !2414
  br i1 %0, label %if.then.i, label %runtime.getTaskStatePtr.exit, !dbg !2415

if.then.i:                                        ; preds = %entry
  call fastcc void @runtime.blockingPanic(), !dbg !2416
  unreachable

runtime.getTaskStatePtr.exit:                     ; preds = %entry
  %1 = call fastcc %runtime.taskState* @"(*runtime.task).state"(i8* nonnull %parentHandle), !dbg !2417
  %2 = bitcast %runtime.taskState* %1 to i8*, !dbg !2417
  call void @runtime.trackPointer(i8* nonnull %2, i8* undef, i8* null), !dbg !2417
  %3 = getelementptr inbounds %runtime.taskState, %runtime.taskState* %1, i32 0, i32 1, !dbg !2418
  %4 = load i8*, i8** %3, align 4, !dbg !2418
  %.spill.addr = getelementptr inbounds %"github.com/aykevl/go-smartwatch.Open.Frame", %"github.com/aykevl/go-smartwatch.Open.Frame"* %FramePtr, i32 0, i32 5, !dbg !2418
  store i8* %4, i8** %.spill.addr, !dbg !2418
  call void @runtime.trackPointer(i8* %4, i8* undef, i8* null), !dbg !2418
  %complit55 = alloca %machine.PinConfig, align 8, !dbg !2408
  %complit48 = alloca %machine.PinConfig, align 8, !dbg !2408
  %complit37 = alloca %"tinygo.org/x/drivers/st7789.Config", align 8, !dbg !2408
  %complit30 = alloca %machine.PinConfig, align 8, !dbg !2408
  %complit = alloca i64, align 8, !dbg !2408
  %tmpcast = bitcast i64* %complit to %machine.SPIConfig*, !dbg !2408
  %spi = alloca %machine.SPI, align 8, !dbg !2408
  %5 = load %"github.com/aykevl/go-smartwatch.Watch"*, %"github.com/aykevl/go-smartwatch.Watch"** @"github.com/aykevl/go-smartwatch.watch", align 4, !dbg !2408
  %6 = bitcast %"github.com/aykevl/go-smartwatch.Watch"* %5 to i8*, !dbg !2408
  call void @runtime.trackPointer(i8* %6, i8* undef, i8* null), !dbg !2408
  %7 = icmp eq %"github.com/aykevl/go-smartwatch.Watch"* %5, null, !dbg !2419
  br i1 %7, label %if.done, label %if.then, !dbg !2420

Before, it was this IR:

define internal fastcc { %"github.com/aykevl/go-smartwatch.Watch"*, %runtime._interface } @"github.com/aykevl/go-smartwatch.Open"(i8* %context, i8* %parentHandle) unnamed_addr #6 section ".text.github.com/aykevl/go-smartwatch.Open" !dbg !2779 {
entry:
  %task.state = alloca %runtime.taskState, !dbg !2781
  %task.state.i8 = bitcast %runtime.taskState* %task.state to i8*, !dbg !2781
  %task.token = call token @llvm.coro.id(i32 0, i8* %task.state.i8, i8* bitcast ({ %"github.com/aykevl/go-smartwatch.Watch"*, %runtime._interface } (i8*, i8*)* @"github.com/aykevl/go-smartwatch.Open" to i8*), i8* null), !dbg !2781
  %task.size = call i32 @llvm.coro.size.i32(), !dbg !2781
  %task.data = call i8* @runtime.alloc(i32 %task.size, i8* undef, i8* null), !dbg !2781
  call void @runtime.trackPointer(i8* %task.data, i8* undef, i8* null), !dbg !2781
  %task.handle = call i8* @llvm.coro.begin(token %task.token, i8* %task.data) #13, !dbg !2781
  %0 = call i8* @runtime.getTaskStatePtr(i8* %parentHandle, i8* undef, i8* null), !dbg !2781
  %retPtr = bitcast i8* %0 to { %"github.com/aykevl/go-smartwatch.Watch"*, %runtime._interface }*, !dbg !2781
  %complit55 = alloca %machine.PinConfig, !dbg !2781
  %complit48 = alloca %machine.PinConfig, !dbg !2781
  %complit37 = alloca %"tinygo.org/x/drivers/st7789.Config", !dbg !2781
  %complit30 = alloca %machine.PinConfig, !dbg !2781
  %complit = alloca %machine.SPIConfig, !dbg !2781
  %spi = alloca %machine.SPI, !dbg !2781
  %1 = load %"github.com/aykevl/go-smartwatch.Watch"*, %"github.com/aykevl/go-smartwatch.Watch"** @"github.com/aykevl/go-smartwatch.watch", !dbg !2781
  %2 = bitcast %"github.com/aykevl/go-smartwatch.Watch"* %1 to i8*, !dbg !2781
  call void @runtime.trackPointer(i8* %2, i8* undef, i8* null), !dbg !2781
  %3 = icmp ne %"github.com/aykevl/go-smartwatch.Watch"* %1, null, !dbg !2782
  br i1 %3, label %if.then, label %if.done, !dbg !2783

But that (unrelated?) issue shouldn't hold back this PR.

@deadprogram deadprogram requested a review from niaow December 8, 2019 14:52
@aykevl aykevl mentioned this pull request Dec 9, 2019
@deadprogram
Copy link
Member

@justinclift if you have time to try this out... :)

@justinclift
Copy link
Member

Maybe next week, definitely not this week though. 😉

Copy link
Member

@niaow niaow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks ok. Can merge after tested by someone who uses wasm more.

By considering this as a regular transformation, it can be easily
tested.
This avoids problems with goroutines in WebAssembly, and is generally a
good thing. It fixes some cases of the following problem:

    LLVM ERROR: Coroutines cannot handle non static allocas yet
@deadprogram
Copy link
Member

Anything stopping this PR from bring merged now?

@aykevl
Copy link
Member Author

aykevl commented Jan 28, 2020

Yes, this should be mergeable. The first is a refactor (plus small bugfix as indicated above) and the second is a fix for a bug that has been a problem for WebAssembly for some time.

@deadprogram
Copy link
Member

OK, merging then. Thanks!

@deadprogram deadprogram merged commit 519adf3 into dev Jan 28, 2020
@deadprogram deadprogram deleted the wasm-abi-alloca branch January 28, 2020 19:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants