Skip to content
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

Add Features flag for LLVM CPU optimizations #131

Closed
wants to merge 3 commits into from
Closed

Conversation

j7b
Copy link
Contributor

@j7b j7b commented Jan 12, 2019

Allows specifying llvm machine features (otherwise specified with -mattr elsewhere in llvm toolchains) via "features" key in targets JSON configuration. Intended to work around issues with AVR object gen, which appear unresolved (identified at avr-llvm/llvm#216 and elsewhere) but should eventually be resolved and adds potentially useful options for stable targets.

Copy link
Member

@aykevl aykevl left a comment

Choose a reason for hiding this comment

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

Does this fix the problem in AVR? Did you test it?
Nothing against this PR - I think it is useful - but I doubt it helps the AVR backend.

@deadprogram
Copy link
Member

Yes @j7b can you please provide an example of this in use for testing? Thank you.

@j7b
Copy link
Contributor Author

j7b commented Jan 14, 2019

Tinygo emits warnings for invalid features as expected, indicating the bindings work as advertised; it appears the issue for avr targets at llvm 7.0 is high-level optimizations assume the 16-bit ops are valid, it appears the productions that trigger them would be inefficient to the point of general impracticality otherwise.

@aykevl
Copy link
Member

aykevl commented Jan 14, 2019

it appears the issue for avr targets at llvm 7.0 is high-level optimizations assume the 16-bit ops are valid

As far as I know, LLVM wasn't designed with systems in mind where general purpose registers are smaller than pointers, as is the case on AVR (8 bit general purpose registers, 16 bit pointers on most chips). The way that they work around it is by emitting 16-bit pseudo instructions that are later lowered to the actual 8 bits instructions.
This probably doesn't produce very efficient code but changing LLVM in this regard would be a lot of work.

Not sure how this relates to CPU features, though.

@j7b
Copy link
Contributor Author

j7b commented Jan 14, 2019

I think the scope of evaluation for this changeset should be limited to feature specification in targets, as an analog to invocations of llvm toolchain utilities using -mattr, the binding is demonstrated to work as invalid features input produces warnings, the inaccurate featuresets specified for a number of atmel parts and astonishing behavior of the optimizer are in the hands of the llvm project.

@deadprogram
Copy link
Member

@j7b can you please provide an example of use so I can test more completely?

Also, this flag needs to be documented somewhere. How about adding to https://github.com/tinygo-org/tinygo-site/blob/master/content/usage/misc-options.md

Thanks!

@deadprogram deadprogram changed the title Features Add Features flag for LLVM CPU optimizations Jan 16, 2019
@j7b
Copy link
Contributor Author

j7b commented Jan 16, 2019

A brutal test is to make a copy of digispark.json with "features":["-sram"], property added, try to compile blinky1 (or anything with a constant that's not optimized away), compilation will fail when llvm tries to assign the instructions to copy constants to sram.

Where'd "adding a target" go in the docs? That was the only thing I recollect that sort of documented the target jsons.

@aykevl
Copy link
Member

aykevl commented Jan 17, 2019

The original bug that led to this PR is now being worked on: avr-rust/rust-legacy-fork#124

Where'd "adding a target" go in the docs? That was the only thing I recollect that sort of documented the target jsons.

https://github.com/aykevl/tinygo/wiki/Adding-a-new-board

@deadprogram deadprogram changed the base branch from master to dev February 5, 2019 16:40
@aykevl
Copy link
Member

aykevl commented May 28, 2019

I have rebased and merged this in 0ae467d. It is very useful for RISC-V support.

@aykevl aykevl closed this May 28, 2019
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