cranelift / Issue #1303 Complete the transition to Basic ... · git-cranelift

Stream: git-cranelift

Topic: cranelift / Issue #1303 Complete the transition to Basic ...

GitHub (Dec 20 2019 at 17:17):

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.

GitHub (Dec 20 2019 at 17:17):

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.

GitHub (Dec 20 2019 at 18:08):

bjorn3 commented on Issue #1303:

https://github.com/bytecodealliance/cranelift/issues/1159 is not yet fixed.

GitHub (Dec 20 2019 at 18:14):

sstangl commented on Issue #1303:

Thanks, I'll fix that first.

GitHub (Dec 20 2019 at 18:17):

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.

GitHub (Jan 24 2020 at 00:02):

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.

GitHub (Jan 24 2020 at 05:36):

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.

GitHub (Jan 24 2020 at 15:13):

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 -> block
One potential issue is that ebb is used extensively throughout the codebase as a short variable name, and renaming to block 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.

GitHub (Jan 24 2020 at 16:16):

bjorn3 commented on Issue #1303:

Maybe use bb and Bb? Just dropping the e for extended.

GitHub (Jan 24 2020 at 16:20):

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...

GitHub (Jan 24 2020 at 16:30):

eqrion commented on Issue #1303:

I agree, that I prefer Block instead of Bb 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 to basic_block when it's referring to just a Block.

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.

GitHub (Jan 24 2020 at 17:01):

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 a BasicBlock, which is a sub-component of an Ebb. It would probably be confusing to have both Block and BasicBlock, so even in the interim, maybe you might rename the latter to something like EbbBlock? That would make it clear that it's a legacy thing.

GitHub (Jan 24 2020 at 20:20):

evilpie commented on Issue #1303:

I think clang uses BB, which I think is better than Bb.

GitHub (Jan 24 2020 at 20:20):

evilpie edited a comment on Issue #1303:

Clang uses BB, which I think is better than Bb.

GitHub (Feb 13 2020 at 22:59):

jyn514 commented on Issue #1303:

I think this has been finished with #1365, is there anything else that needs to be done?

GitHub (Feb 13 2020 at 23:42):

abrown commented on Issue #1303:

I think not; I'll close and we can re-open if anyone finds anything else.

GitHub (Feb 13 2020 at 23:42):

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: Nov 22 2024 at 16:03 UTC