Skip to content
This repository was archived by the owner on Jun 26, 2020. It is now read-only.

cretonne-faerie: add a translation mechanism for LibCalls#321

Merged
sunfishcode merged 7 commits intobytecodealliance:masterfrom
pchickey:pch/faerie-libcall-translation
May 3, 2018
Merged

cretonne-faerie: add a translation mechanism for LibCalls#321
sunfishcode merged 7 commits intobytecodealliance:masterfrom
pchickey:pch/faerie-libcall-translation

Conversation

@pchickey
Copy link
Copy Markdown
Contributor

@pchickey pchickey commented May 2, 2018

Exposes a new field libcall_names: Vec<String> in FaerieBuilder, and a new method FaerieBuilder::default_libcall_names to provide sensible default values for those libcalls.

The FaerieTrapSink looks up any libcall in the names provided, imports that function, and makes a link to it, instead of panicking on encountering a libcall.

"truncf".to_owned(),
"trunc".to_owned(),
"roundf".to_owned(),
"round".to_owned(),
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

These should use nearbyint/nearbyintf rather than round/roundf, as the latter rounds ties away from zero.

(nearbyint/nearbyintf technically depend on the current rounding mode, but this is true of other floating-point operators as well, so for now we can assume that the rounding mode is set to the usual round-to-nearest, ties-to-even)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I didn't know that.

/// representation. A function by this name is imported into the object as part of the
/// translation of a `ir::ExternalName::LibCall` variant. Calls to a LibCall should only be
/// inserted into the IR by the `cretonne_codegen` legalizer pass.
pub fn default_libcall_names() -> Vec<String> {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I know using an array was my suggestion, but looking at it here, I'm worried that it's somewhat fragile, if we ever reorder the libcalls. Would you mind going with your idea, to pass in a closure, instead?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I made this change. Its the first time I dealt with closure lifetimes like this so I'm not 100% sure I got it right, please let me know.

@pchickey pchickey force-pushed the pch/faerie-libcall-translation branch from 0a625cc to 0d4696e Compare May 2, 2018 22:11
pchickey added 2 commits May 2, 2018 15:37
mypy released 0.600 today and even with `--no-strict-optional` flag
passed to it (in lib/codegen/meta/check.sh) it fails to infer the type
of values that did not need type annotations in the past
@pchickey
Copy link
Copy Markdown
Contributor Author

pchickey commented May 2, 2018

The installation of mypy was pinned to version 0.521. The latest version, 0.600, encounters a bunch of errors at the moment. Detailed in bug #323

sunfishcode
sunfishcode approved these changes May 2, 2018
ir::LibCall::TruncF32 => "truncf".to_owned(),
ir::LibCall::TruncF64 => "trunc".to_owned(),
ir::LibCall::NearestF32 => "nearbyint".to_owned(),
ir::LibCall::NearestF64 => "nearbyintf".to_owned(),
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for making this change. As we get more experience with this, it may want to evolve, but this looks good for now.

@sunfishcode sunfishcode merged commit e9eff90 into bytecodealliance:master May 3, 2018
@pchickey pchickey deleted the pch/faerie-libcall-translation branch May 3, 2018 15:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants