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

Change control flow generation scheme #114

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

Conversation

mskvortsov
Copy link
Collaborator

No description provided.

Copy link
Contributor

@akiramenai akiramenai left a comment

Choose a reason for hiding this comment

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

LGTM, but I'd prefer @azhogin to look at the patch as well

const Function &F = MF->getFunction();
if (F.hasFnAttribute("tvm_raw_func")) {
OutStreamer->EmitRawText("\t.internal\t:" + CurrentFnSym->getName());
if (Blocks < 16) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we avoid the magic number here?

// |
// ...
bool TVMIfConversionTerm::tryConvertIf2(MachineBasicBlock *MBB) {
MachineBasicBlock *TBB = nullptr;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpicking, but TrueBB and FalseBB are easier to understand.

Copy link
Contributor

Choose a reason for hiding this comment

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

TBB/FBB are usual names in llvm for true/false destination blocks.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, they are indeed usual. But 'usual' doesn't imply anything on the quality of the choice. Anyway, naming here is up to the author, I merely expressed my opinion.

@azhogin
Copy link
Contributor

azhogin commented Mar 30, 2020

I had an idea to represent function adresses as continuations. And add continuation type to language like slice/builder/cell/tuple.
Would it be easier/harder to support function addresses as continuations with this path accepted?

@mskvortsov
Copy link
Collaborator Author

The idea is surely eligible for the kind of machine you have. Though I'm not sure that LLVM is a right framework for trying to implement the idea.

To answer your question: it would be slightly harder, I guess. But does it matter? What is the example of a function address usage in a code of a contract...

@mskvortsov
Copy link
Collaborator Author

What actually bothers me about this approach is a support of high-level control-flow primitives like WHILE, UNTIL, REPEAT...

@akiramenai
Copy link
Contributor

akiramenai commented Mar 31, 2020

@mskvortsov you've mentioned that the approach you use in the PR doesn't work well with huge functions (with more than 255 BBs). Do you plan to modify the solution somehow (e.g. use tuples of continuations) or do you want to abandon the idea?

@mskvortsov
Copy link
Collaborator Author

From what I see from performance analysis of a certain C++ sample, a huge number of basic blocks (301 in that code to be precise) looks like a problem by itself. Here is an execution statistics of logically the same submitTransaction method coded in C (15k of gas) compared to C++ (24k of gas).
C:

('comparison', 394, '2.62', '%')
('const_literal', 1632, '10.9', '%')
('dictionary', 1391, '9.25', '%')
('application', 1826, '12.1', '%')
('cell', 4237, '28.2', '%')
('codepage_primitives', 52, '0.346', '%')
('control_flow', 470, '3.13', '%')
('tuples', 98, '0.652', '%')
('exceptions', 222, '1.48', '%')
('debug', 0, '0', '%')
('implicit', 1035, '6.88', '%')
('arithmetic', 152, '1.01', '%')
('stack_mapulation', 3524, '23.4', '%')
('total ', 15033)

C++:

('comparison', 696, '2.91', '%')
('const_literal', 3531, '14.8', '%')
('dictionary', 1266, '5.3', '%')
('application', 1972, '8.25', '%')
('cell', 4729, '19.8', '%')
('codepage_primitives', 52, '0.218', '%')
('control_flow', 712, '2.98', '%')
('tuples', 362, '1.51', '%')
('exceptions', 318, '1.33', '%')
('debug', 0, '0', '%')
('implicit', 2530, '10.6', '%')
('arithmetic', 206, '0.862', '%')
('stack_mapulation', 7530, '31.5', '%')
('total ', 23904)

Stack manipulation primitives, implicit jmprefs and rets, and literals add roughly 7.5k of gas. My current guess it's because of stack modelling goes nuts on this huge number of BBs.

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