-
Notifications
You must be signed in to change notification settings - Fork 481
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
Optimization refactoring: VarsInfo and BuiltinsInfo, pure conapps #5551
Conversation
@@ -39,7 +39,7 @@ | |||
!equalsInteger : integer -> integer -> Bool | |||
= \(x : integer) (y : integer) -> | |||
ifThenElse {Bool} (equalsInteger x y) True False | |||
~`$fOrdInteger` : Ord integer | |||
!`$fOrdInteger` : Ord integer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now we know that the dictionary constructor is pure, so it's safe to strictify this!
@@ -312,62 +312,63 @@ | |||
in | |||
let | |||
~`$dOrd` : Ord (List integer) | |||
= let | |||
!v : Ord integer = `$fOrdInteger` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this got inlined thanks to the above strictification
in | ||
Eqv (Sym 1) dt) | ||
dt) | ||
(Eqv (Sym 1) (Sym 1)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All these conapps were pure and singly used, so we inline lots here
!arg : data | ||
= headList {data} (sndPair {integer} {list data} tup) | ||
in | ||
Finite {a} (`$dUnsafeFromData` arg)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now we know that Finite
is strict, so arg
is the first possibly-effectful thing evaluated here, so we can inline it!
let | ||
!eta : List integer = Nil {integer} | ||
in | ||
go eta |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
first-effectful-term firing again
/\dead -> | ||
Maybe_match | ||
{integer} | ||
y |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
y is singly used and now known to be pure, so we inline and COKC fires!
f5fcf25
to
d937cb1
Compare
| DatatypeTyVar { dtNumTyVars :: Int, dtConstructors :: [name] } | ||
|
||
data VarInfo tyname name = | ||
NormalVar { varStrictness :: Strictness, varArity :: Maybe Arity } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we do without these partial field accessors?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pattern matching is much nicer with them... Wish we could drop 8.10 so I can use NoFieldSelectors
... but sure
-- in this case! | ||
(splitApplication -> (h@(Var _ n), args)) | ||
| Just (DatatypeConstructor{}) <- lookupVarInfo n vinfo -> | ||
evalThis (EvalTerm Pure MaybeWork h) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why MaybeWork
? Does a constructor application do any non-trivial work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unclear - I'm generally unclear on what "work free" should actually mean. At the moment the only things that are "work free" are immediately returning a value, which a constructor application certainly does not do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I thought h
is the whole constructor application. If it is the head - the constructor itself, then shouldn't it always be WorkFree?
Also, shouldn't there be an evalThis (EvalTerm Pure MaybeWork t)
after goAppCtx args
? Because t
is the constructor application, and t
may not be workfree like you said.
Btw, Unknown
seems redundant - can we use EvalTerm Impure MaybeWork
in place of Unknown
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, shouldn't there be an evalThis (EvalTerm Pure MaybeWork t) after goAppCtx args?
Nvm about this, I see you just added it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it is the head - the constructor itself, then shouldn't it always be WorkFree?
Unsure: this is the same as we do for variables. Is accessing a variable work-free? You have to look it up... maybe that counts as work? IDK, I just stuck to what was there.
Btw, Unknown seems redundant - can we use EvalTerm Impure MaybeWork in place of Unknown?
Yes, that would be a slightly different way of looking at it. Unknown has two purposes: it tells us that the evaluation goes somewhere unknown, which lets us distinguish terms for which we can see the whole evaluation order with ones where we actually just don't know what happens. If we only care about purity/work, then we can approximate that with "something that is maybe impure and does work", but I think it's kind of nice to say that it's an unknown jump. Also, it's what we use to terminate the traversal: we don't do any more stuff after we hit an Unknown, which is what stops us traversing the whole term in that case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although even if we didn't stop the traversal, if we were lazy enough then EvalTerm Impure MaybeWork
should stop isPure
and isWorkFree
, so those should not traverse the whole term. Not sure, maybe that would be better 🤔
|
||
data VarInfo tyname name = | ||
NormalVar { varStrictness :: Strictness, varArity :: Maybe Arity } | ||
| DatatypeConstructor { dcArity :: Arity , dcParentTyname :: tyname } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need some Haddock explaining what the "parent" of a constructor or a matcher means.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I get rid of the field names it will be even more obscure :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added haddock anyway
e77e1df
to
dded1b8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
…tersectMBO#5551) * Add VarInfo pass, use it everywhere * Add BuiltinsInfo type, use it everywhere * Strictify bindings uses VarsInfo * LetFloatOut uses VarsInfo * ThunkRecursions uses VarsInfo * LetFloatIn uses VarsInfo * Conapps are pure * Changelog * Update 9.6 test output * Fix pir * More fixups?? * Comments
This is some preparatory refactoring for doing case-of-case:
ifThenElse
is a matcher-like function that should participate in case-of-case.The actual optimization changes come from making more passes use variable info (which they should!) and making conapps pure (which they are!). It's a bit of a wash: some things are a little better, some things are a little worse, but the PIR looks better to me, and I think it's better to have accurate information.
Pre-submit checklist: