cfei: don't generate if frame size is zero

[sudhackar]

Description

This small change fixes #5580 as described - only needed 1 additional change and small test fix

Ping @xunilrj

Checklist

• I have linked to any relevant issues.
• I have commented my code, particularly in hard-to-understand areas.
• I have updated the documentation where relevant (API docs, the reference, and the Sway book).
• I have added tests that prove my fix is effective or that my feature works.
• I have added (or requested a maintainer to add) the necessary Breaking* or New Feature labels where relevant.
• I have done my best to ensure that my PR adheres to the Fuel Labs Code Review Standards.
• I have requested a review from the relevant team or maintainers.

[sudhackar]

the failure looks related - it fails on local too but not on master. not flaky. will check and fix

[vaivaswatha]

Hold on merging this. I vaguely remember there being a reason for it's existence. I'll update back soon.

[vaivaswatha]

There's an assumption here in the spiller that the cfei instruction always exists. It's hard for the spiller to know where to insert a new cfei, so it just edits the existing one by growing it by the total spill size.

[sudhackar]

hmm.

Some tests seem to pass with this change

diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/unit_tests/workspace_test/contract_multi_test/src/main.sw b/test/src/e2e_vm_tests/test_programs/should_pass/unit_tests/workspace_test/contract_multi_test/src/main.sw
index b97f32603..7cdc9a8d0 100644
--- a/test/src/e2e_vm_tests/test_programs/should_pass/unit_tests/workspace_test/contract_multi_test/src/main.sw
+++ b/test/src/e2e_vm_tests/test_programs/should_pass/unit_tests/workspace_test/contract_multi_test/src/main.sw
@@ -17,7 +17,7 @@ fn test_foo() {

 #[test(should_revert)]
 fn test_fail() {
-    let contract_id = 0xa5cd13d5d8ceaa436905f361853ba278f6760da2af5061ec86fe09b8a0cf59b4;
+    let contract_id = CONTRACT_ID;
     let caller = abi(MyContract, contract_id);
     let result = caller.test_function {}();
     assert(result == false)
@@ -25,7 +25,7 @@ fn test_fail() {

 #[test]
 fn test_success() {
-    let contract_id = 0xa5cd13d5d8ceaa436905f361853ba278f6760da2af5061ec86fe09b8a0cf59b4;
+    let contract_id = CONTRACT_ID;
     let caller = abi(MyContract, contract_id);
     let result = caller.test_function {}();
     assert(result == true)

contract_id for some reason has been hardcoded here.

So we eventually break here

Testing should_pass/unit_tests/regalloc_spill ... failed
     Compiler panic
          Compiling should_pass/unit_tests/regalloc_spill ...
           Compiling script regalloc_spill (/home/sudhackar/personal/sway/test/src/e2e_vm_tests/test_programs/should_pass/unit_tests/regalloc_spill)
          thread 'main' panicked at sway-core/src/asm_generation/fuel/register_allocator.rs:729:31:
          Function does not have CFEI instruction for locals
          note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

We should close this if needed @vaivaswatha

[IGI-111]

We actually want the contract ids hardcoded in those tests. They're meant to detect possible ABI breakage, using CONTRACT_ID would render them trivial.

Please replace the hardcoded value and mark the PR with the breaking label instead.

[xunilrj]

There's an assumption ... in the spiller that the cfei instruction always exists. It's hard for the spiller to know where to insert a new cfei, so it just edits the existing one by growing it by the total spill size.

That makes sense... but what about after the register allocation?

We can move this check to AllocatedOps::to_fuel_asm at sway-core/src/asm_lang/allocated_ops.rs. This methods already allows a AllocatedOp to return a Vec<FuelOpcode>, so we could do:

 CFEI(a) if a.is_zero() => return Left(vec![]),
 CFEI(a) => op::CFEI::new(a.value.into()).into(),

[vaivaswatha]

There's an assumption ... in the spiller that the cfei instruction always exists. It's hard for the spiller to know where to insert a new cfei, so it just edits the existing one by growing it by the total spill size.

That makes sense... but what about after the register allocation?

We can move this check to AllocatedOps::to_fuel_asm at sway-core/src/asm_lang/allocated_ops.rs. This methods already allows a AllocatedOp to return a Vec<FuelOpcode>, so we could do:

 CFEI(a) if a.is_zero() => return Left(vec![]),
 CFEI(a) => op::CFEI::new(a.value.into()).into(),

This is acceptable. Good idea !.

[sudhackar]

Will follow this feedback and get back.

[sudhackar]

CFEI(a) if a.is_zero() => return Left(vec![]),
CFEI(a) => op::CFEI::new(a.value.into()).into(),

the zero path is triggered quite a few times. This change effectively means we are deleting a line - which could invalidate relations between AllocatedOp and CompiledBytecode such as offsets - triggering even simple tests vec_set_index_out_of_bounds to fail.

Also see

sway/sway-core/src/asm_generation/finalized_asm.rs

Lines 107 to 113 in 996a351

	if ops.len() & 1 != 0 {
	tracing::info!("ops len: {}", ops.len());
	return Err(handler.emit_err(CompileError::Internal(
	"Non-word-aligned (odd-number) ops generated. This is an invariant violation.",
	Span::new(" ".into(), 0, 0, None).unwrap(),
	)));
	}

Deleting a line after this would create a problem.

I confirmed that

            CFEI(a) if a.value == 0 => {
                dbg!(("CFEI with value 0", &a));
                op::NOOP::new().into()
            },

instead of deleting - replacing with a NOOP would pass the current tests.

Is there something that I'm missing?

[sudhackar]

Can I get some traction here?

[vaivaswatha]

Thank you @sudhackar , and apologies for the delay in my response.

replacing with a NOOP would pass the current tests.

But what do we gain at all by replacing it with a NOOP? The code size remains the same. @xunilrj @IGI-111 any thoughts on this?

which could invalidate relations between AllocatedOp and CompiledBytecode such as offsets

I should've seen this coming. There's one more trick you can try. In the function sway_core::asm_generation::fuel::allocated_abstract_instruction_set::AllocatedAbstractInstructionSet::instruction_size, if you return the size of cfei 0 and cfsi 0 to 0, then it wouldn't be counted when computing offsets. That way when you actually omit emitting the instruction (instead of a NOOP), the offsets should all add up. If that doesn't work (for some unforeseen reason) we'll go with the current change.

[sudhackar]

thanks @vaivaswatha for the details - right now with this change - the IR will still generate cfei i0 and cfsi i0 but while the asm generation - these instructions will be removed.

There are no explicit test cases for this change - I'll try to add this. I have added an explicit cfei i0 check in the IR generation just to highlight this.

[vaivaswatha]

Thank you @sudhackar