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.

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

replace STSFLD with PUSH const #1203

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

Conversation

Hecate2
Copy link
Contributor

@Hecate2 Hecate2 commented Oct 12, 2024

We have been always doing STSFLD in _initialize method for all static variables. They can be replaced with PUSH consts when the static variables are read. The major changes are in Peephole.cs

Future steps:

  • For expensive PUSHDATA instructions, if the pushed static variable is read very often, store it to static field at the beginning of method.

@steven1227
Copy link
Member

Nice step

@shargon
Copy link
Member

shargon commented Oct 12, 2024

Love it!!!

@shargon
Copy link
Member

shargon commented Oct 12, 2024

I have different concerns:

  • There are cases where it can be more expensive, some pushes are more expensive. Can we ensure that we choose the cheap one?
  • the script will be bigger, we have a limmit script size. If this limit is reached, how can we avoid this mechanism?
  • whats happene in this case:
class myClass
{
public int MyVar { get; } = 7 ;
public myClass()
   {
   MyVar = 3;
   }
}

@Hecate2
Copy link
Contributor Author

Hecate2 commented Oct 12, 2024

    public class MyClass : SmartContract.Framework.SmartContract
    {
        public static int MyVar = 7;
        public MyClass()
        {
            MyVar = 3;
        }
        public int Main() => MyVar;
    }
# Method Start Neo.Compiler.CSharp.TestContracts.MyClass.Main()
00 INITSLOT 00-01 # 0 local variables, 1 arguments
# Code test-compiler.cs line 13: "MyVar"
03 LDSFLD0
# Method End Neo.Compiler.CSharp.TestContracts.MyClass.Main()
04 RET
# Method Start Neo.Compiler.CSharp.TestContracts.MyClass.MyClass()
05 INITSLOT 00-01 # 0 local variables, 1 arguments
08 LDARG0
09 CALL 05 # pos: 14 (offset: 5)
# Code test-compiler.cs line 11: "3"
# Code test-compiler.cs line 11: "MyVar = 3"
11 PUSH3
# Code test-compiler.cs line 11: "MyVar = 3"
# Code test-compiler.cs line 11: "MyVar = 3"
12 STSFLD0
# Method End Neo.Compiler.CSharp.TestContracts.MyClass.MyClass()
# Code test-compiler.cs line 12: "}"
13 RET
14 INITSLOT 00-01 # 0 local variables, 1 arguments
17 RET
18 INITSSLOT 01 # 1 static variables
20 PUSH7
21 STSFLD0
22 RET
23 NEWARRAY0
24 DUP
25 CALL EC # pos: 5 (offset: -20)
27 JMP E5 # pos: 0 (offset: -27)
29 RET

My optimizer is designed to do nothing on a static variable that is written in the main execution. main executes INITSSLOT at offset 18, and starts at offset 23, and returns 3.

@Hecate2
Copy link
Contributor Author

Hecate2 commented Oct 12, 2024

* There are cases where it can be more expensive, some pushes are more expensive. Can we ensure that we choose the cheap one?
            [OpCode.PUSHINT128] = 1 << 2,
            [OpCode.PUSHINT256] = 1 << 2,
            [OpCode.PUSHDATA1] = 1 << 3,
            [OpCode.PUSHDATA2] = 1 << 9,
            [OpCode.PUSHDATA4] = 1 << 12,

These OpCodes are more expensive than LDSFLD and STSFLD (1 << 1).

For PUSHDATA2 and 4, if they are read twice in a method, it can be more expensive than LDSFLD. I have no immediate solution in a short time, but I have not really seen any contract in production that has to read a single huge piece of static DATA for multiple times. And if such a huge piece of data has to be pushed for every method, other methods become significantly more expensive. Also, developers can write local variables to temporarily store the static variable as a workaround.

For PUSHDATA1, we spend 16+8+2 (INITSSLOT + PUSHDATA1 + STSFLD) to prepare it in the static fields, and get it at a price of 2 (LDSFLD) each time. If we always use PUSHDATA1 when we read it, we can have (16+8+2)/(8-2) = 4.3333 times before it get more expensive than INITSSLOT.

Also we can have (16+4+2)/(4-2) = 11 PUSHINT128s if we do not push it in the _initialize method.

@Jim8y
Copy link
Contributor

Jim8y commented Oct 12, 2024

I have different concerns:

  • There are cases where it can be more expensive, some pushes are more expensive. Can we ensure that we choose the cheap one?
  • the script will be bigger, we have a limmit script size. If this limit is reached, how can we avoid this mechanism?
  • whats happene in this case:
class myClass
{
public int MyVar { get; } = 7 ;
public myClass()
   {
   MyVar = 3;
   }
}

this can be further optimized, for instance do a script size check.

@Jim8y
Copy link
Contributor

Jim8y commented Oct 12, 2024

following this path, you can further optimize static fileds that are only updated and used in one method as the local const variable of that method.

@Jim8y
Copy link
Contributor

Jim8y commented Oct 12, 2024

assign myself @Jim8y a task as updating analyzer to apply relevent check.

@Hecate2
Copy link
Contributor Author

Hecate2 commented Oct 12, 2024

following this path, you can further optimize static fileds that are only updated in one method as the local const variable of that method. cause when i was imolememting out keyword, i use global static variable to store the out value, many things like this can only be done post compile

Complex optimizations require delicate data flow analysis on the nef. Maybe I need to transform all writes (to eval stack, slot and storage) to static single assignment (SSA) form. I may be able to prepare it in half a year.

@Hecate2
Copy link
Contributor Author

Hecate2 commented Oct 15, 2024

* There are cases where it can be more expensive, some pushes are more expensive. Can we ensure that we choose the cheap one?

Is it OK that I do not optimize the static fields that are too expensive to push, and are pushed in a loop or for too many times?

Jim8y
Jim8y previously approved these changes Oct 15, 2024
@Jim8y
Copy link
Contributor

Jim8y commented Oct 15, 2024

@shargon

{
int addr = this.startAddr;
foreach (Instruction i in this.instructions)
if (i.OpCode == opCode && (operand == null || operand.Equals(i.Operand)))
Copy link
Member

Choose a reason for hiding this comment

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

There is no coverage for this method.

Also, maybe something like (!operand.HasValue || operand.Value.Span.SequenceEqual(i.Operand.Span))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. I did plan to use this method but I found it meaningless later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants