[WIP] Add more documentation for entities #1041

jyn514 · 2019-09-17T18:30:24Z

Addresses #1038

Document stackslots, funcrefs, heaps, sigrefs, and jumptables.
Also adds more inter-crate links.

Undocumented

Constant - [WIP] Add more documentation for entities #1041 (comment)
GlobalValue - [WIP] Add more documentation for entities #1041 (comment)
Table - [WIP] Add more documentation for entities #1041 (comment)
Inst - [WIP] Add more documentation for entities #1041 (comment)

Issues

I can't find a way to link to a type/function in a different crate if that crate isn't currently in scope for the code. For example, the documentation for JumpTable links to FunctionBuilder::create_jump_table, but the link is broken (it points to cranelift_codegen/ir/entities/cranelift_frontend::FunctionBuilder::create_jump_table, which is clearly wrong).

There are two ways I can think to resolve this:

Link directly to docs.rs like this: https://docs.rs/cranelift-module/*/cranelift_module/struct.Module.html. This has the downside that it will always point to the latest version (and won't work locally or offline).
Put the name of the type in prose and have users search for it. This is not ideal but will work the same everywhere.

Addresses bytecodealliance#1038 Document stackslots, funcrefs, heaps, sigrefs, and jumptables

abrown · 2019-09-17T18:38:12Z

@jyn514 I can explain Constant: it is an integer used to index into the ConstantPool to identify constant values held there (see https://github.com/CraneStation/cranelift/blob/dcaa7f5ba618d72c9dd24a29c9a485dc24ab0d07/cranelift-codegen/src/ir/constant.rs#L97).

jyn514 · 2019-09-17T18:45:04Z

I see, and it's public because cranelift-frontend needs to access it?

abrown · 2019-09-17T18:46:59Z

I can't find a way to link to a type/function in a different crate if that crate isn't currently in scope for the code

Also, I believe I had similar issues trying to fix some documentation warnings; in the end I went with something like crate::ir::... and that seemed to work: #952. The trick is to use correct Rust paths (see links in https://stackoverflow.com/a/53504254 and rust-lang/rust#43466). I think that getting the Rust paths to work is better long-term than direct links or prose.

jyn514 · 2019-09-17T18:50:48Z

in the end I went with something like crate::ir::...

That only works if the crate is imported into the current scope, though. I can't put extern crate cranelift_module; in the code because that breaks the code of anyone who uses cranelift for WASM.

abrown · 2019-09-17T18:51:35Z

I see, and it's public because cranelift-frontend needs to access it?

Now that I look at its usages, I don't see it anywhere except in codegen so it might be able to be downgraded to pub(crate). The idea, though, is that it is part of a public API for interacting with the constant pool.

cranelift-codegen/src/ir/entities.rs

bjorn3 · 2019-09-17T18:53:24Z

cranelift-codegen/src/ir/entities.rs

+/// [`import_signature`](cranelift_frontend::FunctionBuilder::import_signature).
+///
+/// You can retrieve the [`Signature`](super::Signature) that was used to create a `SigRef` with
+/// [`signature`](cranelift_frontend::FunctionBuilder::signature).


func.dfg.signatures

abrown · 2019-09-17T18:55:25Z

Maybe rust-lang/rust#43466 (comment) is fixed somewhere? Not exactly sure...

abrown · 2019-09-17T19:00:56Z

Maybe rust-lang/rust#43466 (comment) is fixed somewhere? Not exactly sure...

But if there is absolutely no way to link to external crates then I guess my vote would be for the direct link.

- pointer -> reference - mention `stack_store` - mention `Switch` as an alternative to `br_table` - mention `func.dfg.signatures`

can't have two periods in a sentence

abrown · 2019-09-17T19:59:52Z

@jyn514, for Inst you might want to look at how it is used in DataFlowGraph.

bnjbvr · 2019-09-18T08:17:37Z

Thanks for taking a stab at this!

Note that all of these implement EntityRef, so they're all handles to some data living somewhere else. For instance Constant has ConstantData, etc. I remember proposing to constantly suffix them with "Ref" or something, but this was a long time ago and I'm unsure what the outcome of the discussion was.

GlobalValue can be mapped to a wasm global value, but doesn't need to; it is "just" a value that may be live across the entire function lifetime, in my understanding, and that can be preloaded from other global values. (Useful when you have a VmContext, and want to load constant things from it, for instance)
Table is used for wasm function tables at the moment. I don't know if it's planned to use it for other table kinds in the future, could be. Useful for implementing indirect calls to known functions.
Inst is the handle for an instruction. I imagine most usage is internal, and I wonder if this could be made not public.

bjorn3 · 2019-09-18T08:19:42Z

Inst is the handle for an instruction. I imagine most usage is internal, and I wonder if this could be made not public.

I do use it to attach a comment to a specific instruction in the clif ir written by cg_clif.

jyn514 · 2019-09-18T08:24:29Z

Getting slightly off-topic now - is there any way that pretty_clif.rs could be made part of the main Cranelift API? I would love to reuse that code without having to copy/paste most of the implementation.

It looks like the only rustc-specific code is CommentWriter::new, which uses an Instance, let me know if I'm wrong about that.

jyn514 · 2019-09-18T08:37:15Z

Useful when you have a VmContext, and want to load constant things from it

Is this a WASM thing? It mentions a sandbox in the documentation, and I don't see a way to construct it from cranelift_frontend or cranelift_codegen

bjorn3 · 2019-09-18T08:42:14Z

is there any way that pretty_clif.rs could be made part of the main Cranelift API?

#379 (comment)

I would love to reuse that code without having to copy/paste most of the implementation.

Please note that currently comments get inserted after the specified inst, as there is no way to know what the next inst id will be.

jyn514 · 2019-09-18T09:33:47Z

This is ready for review as soon as I fix the broken links. @abrown recommended permalinks to docs.rs, should I do that?

cranelift-codegen/src/ir/entities.rs

abrown · 2019-09-18T15:43:53Z

@jyn514 I'm fine merging this but we need to fix the build issues: https://dev.azure.com/CraneStation/Cranelift/_build/results?buildId=897&view=logs.

jyn514 · 2019-09-18T15:44:53Z

Ok, I'll change it to link directly to docs.rs, then.

See bytecodealliance#1041 (comment), there doesn't currently seem to be a way to link to a type in a dependent crate.

jyn514 · 2019-09-18T23:04:36Z

This is ready for review.

abrown · 2019-09-18T23:49:11Z

@jyn514 thanks for the fix!

…ecodealliance#1041) Also fixes broken links to point to docs.rs; see bytecodealliance/cranelift#1041 (comment), there doesn't currently seem to be a way to link to a type in a dependent crate.

[WIP] Add more documentation for entities

17d18f1

Addresses bytecodealliance#1038 Document stackslots, funcrefs, heaps, sigrefs, and jumptables

bjorn3 suggested changes Sep 17, 2019

View reviewed changes

jyn514 added 3 commits September 17, 2019 15:10

Address review requests

2db2aa8

- pointer -> reference - mention `stack_store` - mention `Switch` as an alternative to `br_table` - mention `func.dfg.signatures`

Document Constant

07e13a1

fix typo

40d4de5

can't have two periods in a sentence

jyn514 added 3 commits September 18, 2019 04:58

document GlobalValues

0d21758

document Tables

7dade6b

document Inst

6d3bcca

bjorn3 reviewed Sep 18, 2019

View reviewed changes

cranelift-codegen/src/ir/entities.rs Outdated Show resolved Hide resolved

link directly to pretty_comment.rs

f716606

bjorn3 approved these changes Sep 18, 2019

View reviewed changes

Fix broken links to point to docs.rs

40b8cfa

See bytecodealliance#1041 (comment), there doesn't currently seem to be a way to link to a type in a dependent crate.

abrown approved these changes Sep 18, 2019

View reviewed changes

abrown merged commit d46311c into bytecodealliance:master Sep 18, 2019

jyn514 deleted the doc-entity branch September 18, 2019 23:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Add more documentation for entities #1041

[WIP] Add more documentation for entities #1041

jyn514 commented Sep 17, 2019 •

edited

Loading

abrown commented Sep 17, 2019

jyn514 commented Sep 17, 2019

abrown commented Sep 17, 2019

jyn514 commented Sep 17, 2019

abrown commented Sep 17, 2019

bjorn3 Sep 17, 2019

abrown commented Sep 17, 2019

abrown commented Sep 17, 2019

abrown commented Sep 17, 2019

bnjbvr commented Sep 18, 2019

bjorn3 commented Sep 18, 2019

jyn514 commented Sep 18, 2019 •

edited

Loading

jyn514 commented Sep 18, 2019

bjorn3 commented Sep 18, 2019

jyn514 commented Sep 18, 2019

abrown commented Sep 18, 2019

jyn514 commented Sep 18, 2019

jyn514 commented Sep 18, 2019

abrown commented Sep 18, 2019

[WIP] Add more documentation for entities #1041

[WIP] Add more documentation for entities #1041

Conversation

jyn514 commented Sep 17, 2019 • edited Loading

Undocumented

Issues

abrown commented Sep 17, 2019

jyn514 commented Sep 17, 2019

abrown commented Sep 17, 2019

jyn514 commented Sep 17, 2019

abrown commented Sep 17, 2019

bjorn3 Sep 17, 2019

Choose a reason for hiding this comment

abrown commented Sep 17, 2019

abrown commented Sep 17, 2019

abrown commented Sep 17, 2019

bnjbvr commented Sep 18, 2019

bjorn3 commented Sep 18, 2019

jyn514 commented Sep 18, 2019 • edited Loading

jyn514 commented Sep 18, 2019

bjorn3 commented Sep 18, 2019

jyn514 commented Sep 18, 2019

abrown commented Sep 18, 2019

jyn514 commented Sep 18, 2019

jyn514 commented Sep 18, 2019

abrown commented Sep 18, 2019

jyn514 commented Sep 17, 2019 •

edited

Loading

jyn514 commented Sep 18, 2019 •

edited

Loading