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.

Sign up for GitHub

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

Jump to bottom

Some getfield related fixes in inference #37423

Closed
wants to merge 1 commit into from
Closed

Some getfield related fixes in inference #37423

wants to merge 1 commit into from

Conversation

yuyichao
Copy link
Contributor

@yuyichao yuyichao commented Sep 6, 2020 •
edited
Loading

  • Use the field index passed in in lift_leaves

    The caller has already done all the computation including bound checking.
    The field computed in this function is also affecting all the following iterations
    which is almost certainly wrong.

  • Remove unnecessary type check on field in lift_leaves since it is always Int

  • Move a branch disabling return nothing higher up

  • Remove some duplicated calculation on field index in getfield_elim_pass!

  • Fix try_compute_fieldidx to return nothing for non-Int Integer field index.

    This can cause getfield_nothrow to return incorrect result.
    It also gives the caller worse type info about the return value.

  • Teach getfield_nothrow that isbits field cannot be undefined and getfield on such field cannot throw.

    This is already handled in isdefined_tfunc.

  • Fix a few wrong use of isbits in dead branches

Ref #26948 (fa02d34)
Ref #27126 (9100329)

@yuyichao yuyichao added compiler:inference Type inference compiler:optimizer Optimization passes (mostly in base/compiler/ssair/) labels Sep 6, 2020
@yuyichao yuyichao requested a review from Keno September 6, 2020 02:39
@yuyichao
Some getfield related fixes in inference
8a6dae4 
* Use the field index passed in in `lift_leaves`

    The caller has already done all the computation including bound checking.
    The `field` computed in this function is also affecting all the following iterations
    which is almost certainly wrong.

* Remove unnecessary type check on `field` in `lift_leaves` since it is always `Int`

* Move a branch disabling `return nothing` higher up

* Remove some duplicated calculation on field index in `getfield_elim_pass!`

* Fix `try_compute_fieldidx` to return `nothing` for non-`Int` `Integer` field index.

    This can cause `getfield_nothrow` to return incorrect result.
    It also gives the caller worse type info about the return value.

* Teach `getfield_nothrow` that `isbits` field cannot be undefined and getfield on such field cannot throw.

    This is already handled in `isdefined_tfunc`.

* Fix a few wrong use of `isbits` in dead branches

----

Ref #26948 (fa02d34)
Ref #27126 (9100329)
@yuyichao yuyichao force-pushed the yyc/typeinf/getfield branch from e6332b8 to 8a6dae4 Compare September 6, 2020 04:13
yuyichao added a commit that referenced this pull request Sep 7, 2020
@yuyichao
Fix getfield_tfunc for constant TypeName
775b5c1 
The field should be either `Symbol` or `Int`. Ref #37423

Unlike #37423, in additional to worse type info in inference,
the missing type check here can actually cause type inference error
due to errors in user code.
yuyichao added a commit that referenced this pull request Sep 7, 2020
@yuyichao
Fix getfield_tfunc for constant TypeName
fc15e71 
The field should be either `Symbol` or `Int`. Ref #37423

Unlike #37423, in additional to worse type info in inference,
the missing type check here can actually cause type inference error
due to errors in user code.
@yuyichao yuyichao mentioned this pull request Sep 7, 2020
Merged
yuyichao added a commit that referenced this pull request Sep 7, 2020
@yuyichao
Fix getfield_tfunc for constant TypeName
c7c418e 
The field should be either `Symbol` or `Int`. Ref #37423

Unlike #37423, in additional to worse type info in inference,
the missing type check here can actually cause type inference error
due to errors in user code.
yuyichao added a commit that referenced this pull request Sep 8, 2020
@yuyichao
Fix getfield_tfunc for constant TypeName
49fb165 
The field should be either `Symbol` or `Int`. Ref #37423

Unlike #37423, in additional to worse type info in inference,
the missing type check here can actually cause type inference error
due to errors in user code.
martinholters pushed a commit that referenced this pull request Sep 8, 2020
@yuyichao
Fix getfield_tfunc for constant TypeName (#37443)
1378bb6 
The field should be either `Symbol` or `Int`. Ref #37423

Unlike #37423, in additional to worse type info in inference,
the missing type check here can actually cause type inference error
due to errors in user code.
KristofferC pushed a commit that referenced this pull request Sep 8, 2020
@yuyichao @KristofferC
Fix getfield_tfunc for constant TypeName (#37443)
f529e8a 
The field should be either `Symbol` or `Int`. Ref #37423

Unlike #37423, in additional to worse type info in inference,
the missing type check here can actually cause type inference error
due to errors in user code.

(cherry picked from commit 1378bb6)
KristofferC pushed a commit that referenced this pull request Sep 8, 2020
@yuyichao @KristofferC
Fix getfield_tfunc for constant TypeName (#37443)
bb3ff89 
The field should be either `Symbol` or `Int`. Ref #37423

Unlike #37423, in additional to worse type info in inference,
the missing type check here can actually cause type inference error
due to errors in user code.

(cherry picked from commit 1378bb6)
JeffBezanson pushed a commit that referenced this pull request Sep 9, 2020
@yuyichao @JeffBezanson
Fix getfield_tfunc for constant TypeName (#37443)
63aa793 
The field should be either `Symbol` or `Int`. Ref #37423

Unlike #37423, in additional to worse type info in inference,
the missing type check here can actually cause type inference error
due to errors in user code.

(cherry picked from commit 1378bb6)
KristofferC pushed a commit that referenced this pull request Sep 10, 2020
@yuyichao @KristofferC
Fix getfield_tfunc for constant TypeName (#37443)
f2e48f8 
The field should be either `Symbol` or `Int`. Ref #37423

Unlike #37423, in additional to worse type info in inference,
the missing type check here can actually cause type inference error
due to errors in user code.

(cherry picked from commit 1378bb6)
Keno
Keno reviewed Dec 29, 2020
View reviewed changes
base/compiler/ssair/passes.jl
@@ -342,22 +340,22 @@ function lift_leaves(compact::IncrementalCompact, @nospecialize(stmt),
else
typ = compact_exprtype(compact, leaf)
if !isa(typ, Const)
# Disabled since #27126
return nothing
Copy link
Member

Choose a reason for hiding this comment

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

I guess I should probably get back to figuring out why this broke.

@Keno Keno mentioned this pull request Dec 30, 2020
Merged
@Keno
Copy link
Member

Keno commented Dec 30, 2020

Merged in #39036.

@Keno Keno closed this Dec 30, 2020
@DilumAluthge DilumAluthge deleted the yyc/typeinf/getfield branch March 25, 2021 22:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Keno Keno Keno approved these changes

Assignees
No one assigned
Labels
compiler:inference Type inference compiler:optimizer Optimization passes (mostly in base/compiler/ssair/)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@yuyichao @Keno