sstangl opened Issue #1303 (assigned to sstangl):
Basic blocks have been default-enabled for a while. Speaking with @bnjbvr, the project has decided to keep moving forward on replacing Ebbs with Blocks.
The remaining points of work to finish that transition are:
- [ ] Removing the "basic-blocks" feature-gate on the relevant code.
- [ ] Mass-renaming "ebb" to "block", etc., throughout the code and the documentation.
However, I think we should keep something that is effectively Ebbs as an internal detail for the wasm IR to CLIF IR translation. They create a de-facto list of blocks that are jump targets, which is a useful optimization for the efficient algorithm we use to convert to SSA.
sstangl assigned Issue #1303 (assigned to sstangl):
Basic blocks have been default-enabled for a while. Speaking with @bnjbvr, the project has decided to keep moving forward on replacing Ebbs with Blocks.
The remaining points of work to finish that transition are:
- [ ] Removing the "basic-blocks" feature-gate on the relevant code.
- [ ] Mass-renaming "ebb" to "block", etc., throughout the code and the documentation.
However, I think we should keep something that is effectively Ebbs as an internal detail for the wasm IR to CLIF IR translation. They create a de-facto list of blocks that are jump targets, which is a useful optimization for the efficient algorithm we use to convert to SSA.
bjorn3 commented on Issue #1303:
https://github.com/bytecodealliance/cranelift/issues/1159 is not yet fixed.
sstangl commented on Issue #1303:
Thanks, I'll fix that first.
sstangl edited Issue #1303 (assigned to sstangl):
Basic blocks have been default-enabled for a while. Speaking with @bnjbvr, the project has decided to keep moving forward on replacing Ebbs with Blocks.
The remaining points of work to finish that transition are:
- [ ] Fixing https://github.com/bytecodealliance/cranelift/issues/1159 (thanks @bjorn3)
- [ ] Removing the "basic-blocks" feature-gate on the relevant code.
- [ ] Mass-renaming "ebb" to "block", etc., throughout the code and the documentation.
However, I think we should keep something that is effectively Ebbs as an internal detail for the wasm IR to CLIF IR translation. They create a de-facto list of blocks that are jump targets, which is a useful optimization for the efficient algorithm we use to convert to SSA.
eqrion edited Issue #1303 (assigned to sstangl):
Basic blocks have been default-enabled for a while. Speaking with @bnjbvr, the project has decided to keep moving forward on replacing Ebbs with Blocks.
The remaining points of work to finish that transition are:
- [x] Fixing https://github.com/bytecodealliance/cranelift/issues/1159 (thanks @bjorn3)
- [ ] Removing the "basic-blocks" feature-gate on the relevant code.
- [ ] Mass-renaming "ebb" to "block", etc., throughout the code and the documentation.
However, I think we should keep something that is effectively Ebbs as an internal detail for the wasm IR to CLIF IR translation. They create a de-facto list of blocks that are jump targets, which is a useful optimization for the efficient algorithm we use to convert to SSA.
sstangl edited Issue #1303 (assigned to sstangl):
Basic blocks have been default-enabled for a while. Speaking with @bnjbvr, the project has decided to keep moving forward on replacing Ebbs with Blocks.
The remaining points of work to finish that transition are:
- [x] Fixing https://github.com/bytecodealliance/cranelift/issues/1159 (thanks @bjorn3)
- [x] Removing the "basic-blocks" feature-gate on the relevant code.
- [ ] Mass-renaming "ebb" to "block", etc., throughout the code and the documentation.
However, I think we should keep something that is effectively Ebbs as an internal detail for the wasm IR to CLIF IR translation. They create a de-facto list of blocks that are jump targets, which is a useful optimization for the efficient algorithm we use to convert to SSA.
eqrion commented on Issue #1303:
I'm going write a commit to do the mass rename now. I believe the plan is to rename to
Block
.So for example:
Ebb -> Block EbbNode -> BlockNode EbbData -> BlockData ebb -> blockOne potential issue is that
ebb
is used extensively throughout the codebase as a short variable name, and renaming toblock
will increase the identifier length by two. In my opinion this could increase readability, but might be too verbose for some peoples taste.Let me know if anyone has objections or a different proposal for naming.
bjorn3 commented on Issue #1303:
Maybe use
bb
andBb
? Just dropping thee
for extended.
bnjbvr commented on Issue #1303:
Bb
looks particularly weird, hence my suggestion of "block" (we have "value", after all). Somebody on IRC mentioned "blk"; I read it as "bulk", but it's nicely shorter than block, so...
eqrion commented on Issue #1303:
I agree, that I prefer
Block
instead ofBb
for the entity name. Rust's capitalization rules penalize two letter acronyms for types, and it's rare to type out the type name with inference.For variable names though, I'm open to
bb
. I suppose it's kind of weird that technically expands tobasic_block
when it's referring to just aBlock
.I think
blk
is a little ambiguous and I'm a bit averse to shortening names by dropping letters in the middle of an identifier, unless it's an acronym, as I think it hurts readability.
sstangl commented on Issue #1303:
Block
looks good to me. The only thing that hasn't been noted is that there's a thing in the repo already called aBasicBlock
, which is a sub-component of anEbb
. It would probably be confusing to have bothBlock
andBasicBlock
, so even in the interim, maybe you might rename the latter to something likeEbbBlock
? That would make it clear that it's a legacy thing.
evilpie commented on Issue #1303:
I think clang uses
BB
, which I think is better thanBb
.
evilpie edited a comment on Issue #1303:
Clang uses
BB
, which I think is better thanBb
.
jyn514 commented on Issue #1303:
I think this has been finished with #1365, is there anything else that needs to be done?
abrown commented on Issue #1303:
I think not; I'll close and we can re-open if anyone finds anything else.
abrown closed Issue #1303 (assigned to sstangl):
Basic blocks have been default-enabled for a while. Speaking with @bnjbvr, the project has decided to keep moving forward on replacing Ebbs with Blocks.
The remaining points of work to finish that transition are:
- [x] Fixing https://github.com/bytecodealliance/cranelift/issues/1159 (thanks @bjorn3)
- [x] Removing the "basic-blocks" feature-gate on the relevant code.
- [ ] Mass-renaming "ebb" to "block", etc., throughout the code and the documentation.
However, I think we should keep something that is effectively Ebbs as an internal detail for the wasm IR to CLIF IR translation. They create a de-facto list of blocks that are jump targets, which is a useful optimization for the efficient algorithm we use to convert to SSA.
Last updated: Dec 23 2024 at 14:03 UTC