From 562ad740dabd6df67382f10b83f6b0e74d699a98 Mon Sep 17 00:00:00 2001 From: Ayke van Laethem Date: Fri, 9 Aug 2019 18:13:47 +0200 Subject: [PATCH] compiler: make runtime.makeGoroutine AVR compatible Previously it would use a bitcast, which cannot directly be used on AVR because functions live in a different address space on AVR. To fix this, use a ptrtoint/inttoptr pair. This allows testdata/coroutines.go to be compiled, but due to what appears to be an LLVM bug cannot be optimized and codegen'ed: tinygo: /home/ayke/src/github.com/tinygo-org/tinygo/llvm-project/llvm/lib/IR/Constants.cpp:1776: static llvm::Constant *llvm::ConstantExpr::getBitCast(llvm::Constant *, llvm::Type *, bool): Assertion `CastInst::castIsValid(Instruction::BitCast, C, DstTy) && "Invalid constantexpr bitcast!"' failed. This happens as one of the function passes after the TinyGo passes and after the module has been verified so most likely it is a bug somewhere in LLVM. --- compiler/compiler.go | 4 ++-- compiler/goroutine-lowering.go | 40 ++++++++++++++++++++-------------- src/runtime/scheduler.go | 2 +- 3 files changed, 27 insertions(+), 19 deletions(-) diff --git a/compiler/compiler.go b/compiler/compiler.go index 6f25cac7..ed5b8c8d 100644 --- a/compiler/compiler.go +++ b/compiler/compiler.go @@ -1045,9 +1045,9 @@ func (c *Compiler) parseInstr(frame *Frame, instr ssa.Instruction) { // interprocedural optimizations. For example, heap-to-stack // transformations are not sound as goroutines can outlive their parent. calleeType := calleeFn.LLVMFn.Type() - calleeValue := c.builder.CreateBitCast(calleeFn.LLVMFn, c.i8ptrType, "") + calleeValue := c.builder.CreatePtrToInt(calleeFn.LLVMFn, c.uintptrType, "") calleeValue = c.createRuntimeCall("makeGoroutine", []llvm.Value{calleeValue}, "") - calleeValue = c.builder.CreateBitCast(calleeValue, calleeType, "") + calleeValue = c.builder.CreateIntToPtr(calleeValue, calleeType, "") // Get all function parameters to pass to the goroutine. var params []llvm.Value diff --git a/compiler/goroutine-lowering.go b/compiler/goroutine-lowering.go index db04b5fd..aa054e15 100644 --- a/compiler/goroutine-lowering.go +++ b/compiler/goroutine-lowering.go @@ -211,17 +211,25 @@ func (c *Compiler) markAsyncFunctions() (needsScheduler bool, err error) { // Add all callees to the worklist. for _, use := range getUses(f) { - if use.IsConstant() && use.Opcode() == llvm.BitCast { - bitcastUses := getUses(use) - for _, call := range bitcastUses { + if use.IsConstant() && use.Opcode() == llvm.PtrToInt { + for _, call := range getUses(use) { if call.IsACallInst().IsNil() || call.CalledValue().Name() != "runtime.makeGoroutine" { - return false, errors.New("async function " + f.Name() + " incorrectly used in bitcast, expected runtime.makeGoroutine") + return false, errors.New("async function " + f.Name() + " incorrectly used in ptrtoint, expected runtime.makeGoroutine") } } // This is a go statement. Do not mark the parent as async, as // starting a goroutine is not a blocking operation. continue } + if use.IsConstant() && use.Opcode() == llvm.BitCast { + // Not sure why this const bitcast is here but as long as it + // has no uses it can be ignored, I guess? + // I think it was created for the runtime.isnil check but + // somehow wasn't removed when all these checks are removed. + if len(getUses(use)) == 0 { + continue + } + } if use.IsACallInst().IsNil() { // Not a call instruction. Maybe a store to a global? In any // case, this requires support for async calls across function @@ -250,12 +258,12 @@ func (c *Compiler) markAsyncFunctions() (needsScheduler bool, err error) { // goroutine is not async (does not do any blocking operation), no // scheduler is necessary as it can be called directly. for _, use := range getUses(makeGoroutine) { - // Input param must be const bitcast of function. - bitcast := use.Operand(0) - if !bitcast.IsConstant() || bitcast.Opcode() != llvm.BitCast { - panic("expected const bitcast operand of runtime.makeGoroutine") + // Input param must be const ptrtoint of function. + ptrtoint := use.Operand(0) + if !ptrtoint.IsConstant() || ptrtoint.Opcode() != llvm.PtrToInt { + panic("expected const ptrtoint operand of runtime.makeGoroutine") } - goroutine := bitcast.Operand(0) + goroutine := ptrtoint.Operand(0) if _, ok := asyncFuncs[goroutine]; ok { needsScheduler = true break @@ -571,14 +579,14 @@ func (c *Compiler) lowerMakeGoroutineCalls() error { makeGoroutine := c.mod.NamedFunction("runtime.makeGoroutine") for _, goroutine := range getUses(makeGoroutine) { - bitcastIn := goroutine.Operand(0) - origFunc := bitcastIn.Operand(0) + ptrtointIn := goroutine.Operand(0) + origFunc := ptrtointIn.Operand(0) uses := getUses(goroutine) - if len(uses) != 1 || uses[0].IsABitCastInst().IsNil() { - return errors.New("expected exactly 1 bitcast use of runtime.makeGoroutine") + if len(uses) != 1 || uses[0].IsAIntToPtrInst().IsNil() { + return errors.New("expected exactly 1 inttoptr use of runtime.makeGoroutine") } - bitcastOut := uses[0] - uses = getUses(bitcastOut) + inttoptrOut := uses[0] + uses = getUses(inttoptrOut) if len(uses) != 1 || uses[0].IsACallInst().IsNil() { return errors.New("expected exactly 1 call use of runtime.makeGoroutine bitcast") } @@ -593,7 +601,7 @@ func (c *Compiler) lowerMakeGoroutineCalls() error { c.builder.SetInsertPointBefore(realCall) c.builder.CreateCall(origFunc, params, "") realCall.EraseFromParentAsInstruction() - bitcastOut.EraseFromParentAsInstruction() + inttoptrOut.EraseFromParentAsInstruction() goroutine.EraseFromParentAsInstruction() } diff --git a/src/runtime/scheduler.go b/src/runtime/scheduler.go index 9d7bc1de..cd994fd1 100644 --- a/src/runtime/scheduler.go +++ b/src/runtime/scheduler.go @@ -47,7 +47,7 @@ func (t *coroutine) promise() *taskState { return (*taskState)(t._promise(int32(unsafe.Alignof(taskState{})), false)) } -func makeGoroutine(*uint8) *uint8 +func makeGoroutine(uintptr) uintptr // Compiler stub to get the current goroutine. Calls to this function are // removed in the goroutine lowering pass.