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.

Sign up for GitHub

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

Jump to bottom

Introduce GlobalVar akin to LocalVar and introduce get_global instruction similar to get_local. #6928

Open
wants to merge 28 commits into
base: master
Choose a base branch
from
Open

Introduce GlobalVar akin to LocalVar and introduce get_global instruction similar to get_local. #6928

wants to merge 28 commits into from

Conversation

vaivaswatha
Copy link
Contributor

2nd Step as part of #6351, in continuation to #6896

@codspeed-hq CodSpeed HQ
Copy link

codspeed-hq bot commented Feb 14, 2025 •
edited
Loading

CodSpeed Performance Report

Merging #6928 will not alter performance

Comparing vaivaswatha/ir_consts_2 (f3088a8) with master (a090937)

Summary

✅ 22 untouched benchmarks

@vaivaswatha vaivaswatha self-assigned this Feb 14, 2025
@vaivaswatha vaivaswatha changed the base branch from master to vaivaswatha/ir_consts_1 February 19, 2025 06:24
Base automatically changed from vaivaswatha/ir_consts_1 to master February 20, 2025 01:02
@vaivaswatha vaivaswatha marked this pull request as ready for review February 20, 2025 12:09
@vaivaswatha vaivaswatha requested a review from a team as a code owner February 20, 2025 12:09
@ironcev ironcev added compiler General compiler. Should eventually become more specific as the issue is triaged compiler: ir IRgen and sway-ir including optimization passes labels Feb 25, 2025
ironcev
ironcev requested changes Feb 25, 2025
View reviewed changes
sway-core/src/asm_generation/fuel/fuel_asm_builder.rs Outdated
let reg = self.reg_seqr.next();
self.cur_bytecode.push(Op {
opcode: either::Either::Left(VirtualOp::AddrDataId(reg.clone(), data_id.clone())),
comment: "constant address in data section".into(),
Copy link
Member

Choose a reason for hiding this comment

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

Op::comments of other ops follow these guidelines. It would be good to be consistent here, it makes the reading of the printed ASM easier.

Suggested change
comment: "constant address in data section".into(),
comment: "get constant's address in data section".into(),

sway-core/src/ir_generation/const_eval.rs Outdated
if let Some(constant) = global_var.get_initializer(env.context) {
return Ok(Some(Value::new_constant(env.context, *constant)));
}
Ok(None)
Copy link
Member

Choose a reason for hiding this comment

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

Can this actually happen? Shouldn't it actually be an ICE? GlobalVars are modeled as variables, means they do not necessarily need to have an initializer, that is fine. My question is the exact semantics of getting an uninitialized GlobalVar in the context of const_eval. We are compiling a const decl and we are getting a global_var from the env.module. It means it exists, but it doesn't have an initializer.

Doesn't this points to a bug somewhere before in the compilation chain and should be an ICE? Or am I missing some details?

sway-ir/src/parser.rs Outdated
@@ -43,11 +43,12 @@ mod ir_builder {
}

rule script_or_predicate() -> IrAstModule
= kind:module_kind() "{" _ configs:init_config()* _ fn_decls:fn_decl()* "}" _
= kind:module_kind() "{" _ configs:init_config()* _ global_consts:global_const()* _ fn_decls:fn_decl()* "}" _
Copy link
Member

Choose a reason for hiding this comment

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

If we start mixing the terms "global variables/vars" and "global constants/const" it `will become very confusing 😄

The proposal is to stay consistent and use the term "global variable" everywhere including here in the parse. Thus, to rename:

  • global_consts -> global_vars
  • global_const -> global_var
  • IrAstGlobalConst -> IrAstGlobalVar
  • build_global_consts_map -> build_global_vars_map

sway-ir/src/parser.rs Outdated
@@ -982,6 +1010,7 @@ mod ir_builder {
let mut builder = IrBuilder {
module,
configs_map: build_configs_map(&mut ctx, &module, ir_ast_mod.configs, &md_map),
global_map: build_global_consts_map(&mut ctx, &module, ir_ast_mod.global_consts),
Copy link
Member

Choose a reason for hiding this comment

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

Same here.

Suggested change
global_map: build_global_consts_map(&mut ctx, &module, ir_ast_mod.global_consts),
globals_map: build_global_vars_map(&mut ctx, &module, ir_ast_mod.global_vars),

sway-ir/src/parser.rs Outdated
@@ -998,6 +1027,7 @@ mod ir_builder {
struct IrBuilder {
module: Module,
configs_map: BTreeMap<String, String>,
global_map: BTreeMap<Vec<String>, GlobalVar>,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
global_map: BTreeMap<Vec<String>, GlobalVar>,
globals_map: BTreeMap<Vec<String>, GlobalVar>,

@vaivaswatha
Copy link
Contributor Author

@ironcev I've addressed your comments.

@vaivaswatha vaivaswatha requested review from ironcev and a team February 26, 2025 03:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ironcev ironcev Awaiting requested review from ironcev

Requested changes must be addressed to merge this pull request.

Assignees

@vaivaswatha vaivaswatha

Labels
compiler: ir IRgen and sway-ir including optimization passes compiler General compiler. Should eventually become more specific as the issue is triaged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@vaivaswatha @ironcev @IGI-111