Skip to content

Conversation

@chrchr-github
Copy link
Collaborator

This gives a speedup of ~2x, but still scales as O(N^2).

@firewave
Copy link
Collaborator

firewave commented Apr 27, 2023

This is a big win - it saves 15% of total Ir for cli/cmdlineparser.cpp with DISABLE_VALUEFLOW=1.

The underlying run-time can be improved by using Boost (I still have changes utilizing that by default for the Windows release builds) and/or applying #4526.

@danmar If you provide someone with binaries you should build them with USE_BOOST=On.

@pfultz2
Copy link
Contributor

pfultz2 commented Apr 27, 2023

Since isSameExpression is commutative this avoids isSameExpression(y, x) since we already checked isSameExpresion(x, y).

but still scales as O(N^2).

Yea instead of using this nested loop we can first check the tokens that do not have children. When it matches then we can check the astParent and then repeat. If it doesn't match we might still need to check the parent if its the same operator and its commutative(such as +) because isSameExpression handles such cases when the operands are reordered.

@chrchr-github
Copy link
Collaborator Author

1: void f ( ) {
2: const char * a@var1 [@expr5 6 ] ;
3: int i@var2 ; i@var2 =@expr6 0 ;
4: a@var1 [@expr7 i@var2 ++@expr8 ] =@expr9 "a" ;
5: a@var1 [@expr10 i@var2 ++@expr11 ] =@expr12 "b" ;
6: a@var1 [@expr13 i@var2 ++@expr14 ] =@expr15 "c" ;
7: a@var1 [@expr16 i@var2 ++@expr17 ] =@expr18 "d" ;
8: a@var1 [@expr19 i@var2 ++@expr20 ] =@expr21 "e" ;
9: a@var1 [@expr22 i@var2 ++@expr23 ] =@expr24 "f" ;

In this example, there are three expression tokens, [, ++, and =. Under which circumstances would ++ get the same exprid than another ++? Here they are clearly different:

1: void g ( ) {
2: int a@var1 [@expr3 2 ] =@expr4 {@expr5 0 , 0 } ;
3: int i@var2 ; i@var2 =@expr6 0 ;
4: a@var1 [@expr7 i@var2 ++@expr8 ] =@expr9 0 ;
5: i@var2 =@expr10 0 ;
6: a@var1 [@expr11 i@var2 ++@expr12 ] =@expr13 0 ;
7: }

Point being, maybe we can omit some expr tokens?

@pfultz2
Copy link
Contributor

pfultz2 commented Apr 27, 2023

Under which circumstances would ++ get the same exprid than another ++?

Never, although we dont usualy check for side effects, this one seems obvious. Thats why if we start with the leaf nodes in the ast we can then skip operators like ++ and all of its parents(like [ and =).

With the current algorithm, if we skip ++, we will still be checking for same expressions on the [ and =, which will check ++ again(and return false).

@pfultz2
Copy link
Contributor

pfultz2 commented Apr 27, 2023

Yea instead of using this nested loop we can first check the tokens that do not have children. When it matches then we can check the astParent and then repeat.

This is really a suggestion for a future PR. I think this PR can be merged as is.

@firewave firewave merged commit 96cf2b3 into danmar:main Apr 27, 2023
@chrchr-github chrchr-github deleted the chr_Fix10192 branch May 22, 2023 19:02
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.

3 participants