Contracts and contract calls in IR #976

mohammadfawaz · 2022-03-20T22:20:27Z

This change basically enables contract calls for IR. Main changes

Introduce a new IR instruction contract_call to call a contract. E.g.:
v12 = contract_call get_b256<42123b96>{addr: v8, coins: v9, asset_id: v10, gas:v11}(v7)
- note the selector ID, contract address, coins, asset_id, gas, and then the user arguments.
Handle asm generation for contract method arguments on both the caller side and the callee side.

I tested the contract tests manually with --use-ir and they seemed okay.

adlerjohn

Merge conflicts 😬

mohammadfawaz · 2022-03-20T23:03:29Z

Question: the default value for gas is $cgas but it's unclear to me how to represent that in IR. For now, I'm setting the value to zero and interpreting that as $cgas in codegen. That implies that if the user sets gas to 0 in Sway, then $cgas will be forwarded instead of 0. @otrho @adlerjohn thoughts?

mohammadfawaz · 2022-03-20T23:04:33Z

sway-ir/src/instruction.rs

@@ -99,6 +109,7 @@ impl Instruction {
        match self {
            Instruction::AsmBlock(asm_block, _) => asm_block.get_type(context),
            Instruction::Call(function, _) => Some(context.functions[function.0].return_type),
+            Instruction::ContractCall { .. } => None, // TODO fixed this


@otrho thoughts on this? I don't have the type of this here so I probably need to carry it over in the AST. For function call, you've done this by looking at the body of the function definition. For contract calls, we can't really do that.

From the Sway side the call returns a typed value though, which can be determined by the ABI I guess. So we might need to either bundle up the return type into the instruction since we have the selector there, it can be fetched from the ABI.

Note that not all contract calls will have access to the ABI at compile time. E.g. a smart contract wallet that simply calls a provided contract after authentication is not going to know the ABI of all possible contract it will call when it's compiled. ABI casting implies knowing the ABI, but a use of the CALL instruction does not.

adlerjohn · 2022-03-20T23:14:14Z

For now, I'm setting the value to zero and interpreting that as $cgas in codegen. That implies that if the user sets gas to 0 in Sway, then $cgas will be forwarded instead of 0.

You could instead set it to some max value (e.g. u64 max), then in the codegen set it to $cgas?

otrho

This is great, thought my initial impression is that there's too much going on in from_ir.rs and the ContractCall instruction in the IR is too big. I'll discuss this offline with you some more though.

otrho · 2022-03-21T00:37:41Z

sway-ir/src/instruction.rs

@@ -99,6 +109,7 @@ impl Instruction {
        match self {
            Instruction::AsmBlock(asm_block, _) => asm_block.get_type(context),
            Instruction::Call(function, _) => Some(context.functions[function.0].return_type),
+            Instruction::ContractCall { .. } => None, // TODO fixed this


From the Sway side the call returns a typed value though, which can be determined by the ABI I guess. So we might need to either bundle up the return type into the instruction since we have the selector there, it can be fetched from the ABI.

otrho · 2022-03-22T00:58:56Z

I don't want seem authoritarian because 'this isn't how I would do it' so push back if this is unreasonable... but I'd prefer the contract call instruction to resemble the CALL opcode, with just 4 regular parameters.

The coins, asset and gas would just be the 2nd-4th args. The first arg would be a struct pointer which IRgen sets up with the address, selector and user args. Much of the complexity in from_ir.rs: AsmBuilder::compile_contract_call(), which is a 250 line function here, would be relieved.

Having the named special args in the curly braces is a bit out of place and pretty much unnecessary IMO. And the issue around a default for gas would be handled by the compiler -- if it requires the value from the register then it injects an ASM block to fetch it, or even better, we introduce a simple instruction which fetches the register value for us.

mohammadfawaz · 2022-03-22T16:12:01Z

The above has been redesigned. The contract call instruction is now much simpler:
v1 = contract_call v2, v3, v4, v5 where the values match the registers used by the CALL VM instruction.
IRGen will basically create a pointer to a list of parameters (address, selector, and user parameters) and use that in the
contract_call instruction.

An instruction called read_register was also added that allows reading a special register from the VM. For now, we only need $cgas to set the default value for gas in the contract_call instruction. In the future, there may be other uses it read_register.

mohammadfawaz · 2022-03-22T16:17:20Z

sway-ir/tests/ir_to_ir/constants_contract_calls.in_ir

+        v23 = const u64 0
+        v24 = const b256 0x0000000000000000000000000000000000000000000000000000000000000000
+        v25 = const u64 20000
+        v26 = contract_call v22, v23, v24, v25


@otrho should we go with or with something like:
v26 = contract_call method_name(v22, v23, v24, v25)?
We can easily do both. The only downside of the above is that it looks like a regular method call instead of an instruction.

If we want to add the method name for readability then it could just be an ignored identifier parameter, like
_ = contract_call method_name, v22, v23, v24, v25.

Or if we want to add it to be useful for tooling then it could be added as metadata.

otrho

I do much prefer this. Even though the 250 line func in from_ir.rs became a 180 line func in optimize.rs I think it's better to have the complexity there.

A few comments but nothing urgent. Either this or my IR verification PR is going to break the other, though they might still not conflict... might be better to merge this first since I know how to fix the aggregate stuff and can add the verification for these new ops.

otrho · 2022-03-23T02:46:20Z

sway-core/src/asm_generation/from_ir.rs

+
+        self.reg_map.insert(*instr_val, instr_reg);
+
+        ok((), Vec::new(), Vec::new())


There's no need for this to return a CompileResult when it can't fail.

otrho · 2022-03-23T02:52:40Z

sway-ir/src/parser.rs

+            rule op_read_register() -> IrAstOperation
+                = "read_register" _ reg_name:id() {
+                    IrAstOperation::ReadRegister(reg_name)
+                }


I'd be tempted to validate the register names here, rather than just letting them be any id.

otrho · 2022-03-23T02:56:35Z

sway-ir/src/printer.rs

+                    ))
+                    .append(match span_md_idx {
+                        None => Doc::Empty,
+                        Some(_) => Doc::text(md_namer.meta_as_string(context, span_md_idx, true)),


The way I've done the metadata elsewhere is to let md_namer.meta_as_string() return an empty string on None, also letting it add the comma on Some. The match isn't needed at all then.

otrho · 2022-03-23T02:56:59Z

sway-ir/src/printer.rs

+                namer.name(context, ins_value),
+                reg_name,
+                md_namer.meta_as_string(context, span_md_idx, true),
+            )),


Which is exactly what you've done here... 🙂

mohammadfawaz · 2022-03-23T03:09:19Z

I do much prefer this. Even though the 250 line func in from_ir.rs became a 180 line func in optimize.rs I think it's better to have the complexity there.

A few comments but nothing urgent. Either this or my IR verification PR is going to break the other, though they might still not conflict... might be better to merge this first since I know how to fix the aggregate stuff and can add the verification for these new ops.

Thanks Toby. I will merge this now then and will address your comments in a separate PR tomorrow.

mohammadfawaz added 5 commits March 19, 2022 23:02

Initial commit

337da5e

fixing a few things

1e1e9fc

additional fixes

4c91105

adding tests

dbd0e80

fixing default value for cgas

e02ab43

mohammadfawaz requested review from otrho and sezna March 20, 2022 22:20

adlerjohn assigned mohammadfawaz Mar 20, 2022

adlerjohn added enhancement New feature or request compiler General compiler. Should eventually become more specific as the issue is triaged compiler: ir IRgen and sway-ir including optimization passes labels Mar 20, 2022

adlerjohn reviewed Mar 20, 2022

View reviewed changes

mohammadfawaz added 2 commits March 20, 2022 18:28

merge with master

731a78a

did not mean to change this test

4c66aa9

mohammadfawaz commented Mar 20, 2022

View reviewed changes

otrho suggested changes Mar 21, 2022

View reviewed changes

mohammadfawaz added 3 commits March 21, 2022 09:37

Switching the default value for gas in IR to std::u64::MAX

b8eb0f0

fix a test

03f07e0

one last test that I forgot to update

199c382

This was referenced Mar 21, 2022

Implement storage API - part 3 #979

Closed

Implement storage access via storage keyword #646

Merged

mohammadfawaz added 2 commits March 22, 2022 11:58

addressing Toby's comments

0d01389

fmt

ea42c91

mohammadfawaz commented Mar 22, 2022

View reviewed changes

mohammadfawaz added 2 commits March 22, 2022 15:11

remove redundant code for loading cgas

af4f036

fmt

b82beb9

otrho approved these changes Mar 23, 2022

View reviewed changes

mohammadfawaz merged commit dba70a3 into master Mar 23, 2022

mohammadfawaz deleted the mohammadfawaz/contract_call_with_ir branch March 23, 2022 03:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Contracts and contract calls in IR #976

Contracts and contract calls in IR #976

mohammadfawaz commented Mar 20, 2022 •

edited

Loading

adlerjohn left a comment

mohammadfawaz commented Mar 20, 2022

mohammadfawaz Mar 20, 2022

otrho Mar 21, 2022

adlerjohn Mar 21, 2022

adlerjohn commented Mar 20, 2022

otrho left a comment

otrho Mar 21, 2022

otrho commented Mar 22, 2022

mohammadfawaz commented Mar 22, 2022

mohammadfawaz Mar 22, 2022

otrho Mar 23, 2022 •

edited

Loading

otrho left a comment

otrho Mar 23, 2022

otrho Mar 23, 2022

otrho Mar 23, 2022

otrho Mar 23, 2022

mohammadfawaz commented Mar 23, 2022


		self.reg_map.insert(*instr_val, instr_reg);

		ok((), Vec::new(), Vec::new())

Contracts and contract calls in IR #976

Contracts and contract calls in IR #976

Conversation

mohammadfawaz commented Mar 20, 2022 • edited Loading

adlerjohn left a comment

Choose a reason for hiding this comment

mohammadfawaz commented Mar 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adlerjohn commented Mar 20, 2022

otrho left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

otrho commented Mar 22, 2022

mohammadfawaz commented Mar 22, 2022

Choose a reason for hiding this comment

otrho Mar 23, 2022 • edited Loading

Choose a reason for hiding this comment

otrho left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mohammadfawaz commented Mar 23, 2022

mohammadfawaz commented Mar 20, 2022 •

edited

Loading

otrho Mar 23, 2022 •

edited

Loading