Skip to content

compiler: remove ir package #1584

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

Merged
merged 1 commit into from
Jan 24, 2021
Merged

compiler: remove ir package #1584

merged 1 commit into from
Jan 24, 2021

Conversation

aykevl
Copy link
Member

@aykevl aykevl commented Jan 19, 2021

This package was long making the design of the compiler more complicated than it needs to be. Previously this package implemented several optimization passes, but those passes have since moved to work directly with LLVM IR instead of Go SSA. The only remaining pass is the SimpleDCE pass.

This commit removes the *ir.Function type that permeated the whole compiler and instead switches to use *ssa.Function directly. The SimpleDCE pass is kept but is far less tightly coupled to the rest of the compiler so that it can easily be removed once the switch to building and caching packages individually happens.


For two previous attempts at this change, see #1008 and #466. This PR is different in that it only refactors but does not remove the SimpleDCE pass. That avoids the slowdown in the previous two PRs, it is purely a refactor. This PR does change the produced binaries and there are even some size changes but they are rare and look random (sometimes 16 bytes more, sometimes 16 bytes less).

This package was long making the design of the compiler more complicated
than it needs to be. Previously this package implemented several
optimization passes, but those passes have since moved to work directly
with LLVM IR instead of Go SSA. The only remaining pass is the SimpleDCE
pass.

This commit removes the *ir.Function type that permeated the whole
compiler and instead switches to use *ssa.Function directly. The
SimpleDCE pass is kept but is far less tightly coupled to the rest of
the compiler so that it can easily be removed once the switch to
building and caching packages individually happens.
@aykevl aykevl requested a review from niaow January 19, 2021 12:42
@deadprogram
Copy link
Member

I saw that the ItsyBitsy-M4 tests had failed TinyHCI. I retried the HCI a few times, and then tried verifying manually from previous version, which passed:

$ make test-itsybitsy-m4 
tinygo flash -size short -target=itsybitsy-m4 -port=/dev/itsybitsy_m4 ./itsybitsy-m4/
   code    data     bss |   flash     ram
  13172      44    6360 |   13216    6404
Running tests...
./build/testrunner /dev/itsybitsy_m4 115200 5

- digitalReadVoltage = ***pass***
- digitalReadGround = ***pass***
- digitalWriteOn = ***pass***
- digitalWriteOff = ***pass***
- analogReadVoltage = ***pass***
- analogReadGround = ***pass***
- analogReadHalfVoltage = ***pass***
- i2cConnectionNoPower = ***pass***
- i2cConnectionPower = ***pass***

### Tests complete.

Seems like this commit somehow broke I2C on the ItsyBitsy-M4 board.

@aykevl
Copy link
Member Author

aykevl commented Jan 19, 2021

Well, that's unexpected. I did not expect any behavioral changes (especially since all regular tests pass). I'll investigate the issue.

@aykevl
Copy link
Member Author

aykevl commented Jan 20, 2021

Somehow it appears the order of functions is relevant 🤔 When I force declare them all at the beginning, the following binary becomes identical before and after this PR:

tinygo build -o test.hex -target=itsybitsy-m4 -size=short ./examples/mpu6050; and md5sum test.hex

patch:

diff --git a/compiler/compiler.go b/compiler/compiler.go
index 745d2ad2..8c25d874 100644
--- a/compiler/compiler.go
+++ b/compiler/compiler.go
@@ -255,7 +255,10 @@ func CompileProgram(pkgName string, lprogram *loader.Program, machine llvm.Targe
 
        // Predeclare the runtime.alloc function, which is used by the wordpack
        // functionality.
-       c.getFunction(c.program.ImportedPackage("runtime").Members["alloc"].(*ssa.Function))
+       //c.getFunction(c.program.ImportedPackage("runtime").Members["alloc"].(*ssa.Function))
+       for _, f := range functions {
+             c.getFunction(f)
+       }
 
        // Add definitions to declarations.
        var initFuncs []llvm.Value

But I'd like to know why this is relevant.

@aykevl
Copy link
Member Author

aykevl commented Jan 20, 2021

I can reproduce this on a PyBoard with the mpu6050 driver example.

@aykevl
Copy link
Member Author

aykevl commented Jan 20, 2021

Actually, I can't. It doesn't work before this commit either. When I run this (with a change to delay 3 seconds at startup to get the first printed messages):

tinygo flash -target=pybadge ./examples/mpu6050

I get output like this:

connected: false
0 0 0
0 0 0
0 0 0
-316406 29052 987060
-316406 24169 995605
-314697 26855 993652

That's with this PR, with the commit right before this PR (f159429), and on the release branch (and of course without the temporary patch above).

@deadprogram can you maybe test with a PyBadge, whether you can reproduce this behavior? There may be a different/bigger problem here.

@deadprogram
Copy link
Member

@aykevl tested with PyBadge and MPU6050 and it worked exactly as expected with both the latest dev as well as with this branch. No changes to the MPU6050 test program from drivers repo needed.

@deadprogram
Copy link
Member

The following change removes the TinyHCI error: tinygo-org/tinyhci@27b25be

Was using a pointer now just using type. That may or may not actually be significant?

@deadprogram
Copy link
Member

Actually, no. It was just that I had plugged in the board. Appears that the first time it worked, but did not after a soft reset.

@deadprogram
Copy link
Member

I really do not think my m4 adventures are related to this PR, just happened to show up around same time.

@aykevl
Copy link
Member Author

aykevl commented Jan 21, 2021

So, does that mean we can ignore the test failure?

@deadprogram
Copy link
Member

So, does that mean we can ignore the test failure?

Yes.

@deadprogram
Copy link
Member

@niaow this PR is now ready for review.

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 much better

@aykevl
Copy link
Member Author

aykevl commented Jan 24, 2021

@niaow thank you for taking a look!

@aykevl aykevl merged commit d8cc48b into dev Jan 24, 2021
@aykevl aykevl deleted the compiler-remove-ir-package branch January 24, 2021 14:39
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.

3 participants