-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Align meaning for effects and IR flags #50313
Conversation
This fixes a longstanding todo where the IR_FLAG_EFFECT_FREE flag actually required both :effect_free and :nothrow. After this PR, it is equivalent to :effect_free only. The mismatch in meaning here caused #50311. `Symbol(::String)` is :effect_free, but not :nothrow. As a result, setting IR_FLAG_EFFECT_FREE on it was not legal. Later, irinterp did discover that it was nothrow and set IR_FLAG_NOTHROW, but did not have sufficient information to know that it was also :effect_free, so it could not set that flag. With this PR, IR_FLAG_EFFECT_FREE is set early in inference, so once irinterp discovers IR_FLAG_NOTHROW, the call becomes DCE-eligible as desired. Fixes #50311.
@@ -510,9 +510,11 @@ end) | |||
|
|||
function Symbol(s::String) | |||
@_foldable_meta | |||
@noinline |
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 is this @noinline
?
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.
There's rarely a good reason to inline a ccall and in this case if you inline it, you lose the ability to consteval it, because the effect flags only attach to the function as a whole.
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.
@@ -1650,7 +1651,7 @@ function inline_const_if_inlineable!(inst::Instruction) | |||
inst[:inst] = quoted(rt.val) | |||
return true | |||
end | |||
inst[:flag] |= IR_FLAG_EFFECT_FREE | |||
inst[:flag] |= IR_FLAG_EFFECT_FREE | IR_FLAG_NOTHROW |
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.
Maybe better to define
const IR_FLAG_REMOVAL_IF_UNUSED
.
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.
Sure
remove noneffectfree per JuliaLang/julia#50313
This fixes a longstanding todo where the IR_FLAG_EFFECT_FREE flag actually required both :effect_free and :nothrow. After this PR, it is equivalent to :effect_free only. The mismatch in meaning here caused #50311.
Symbol(::String)
is :effect_free, but not :nothrow. As a result, setting IR_FLAG_EFFECT_FREE on it was not legal. Later, irinterp did discover that it was nothrow and set IR_FLAG_NOTHROW, but did not have sufficient information to know that it was also :effect_free, so it could not set that flag. With this PR, IR_FLAG_EFFECT_FREE is set early in inference, so once irinterp discovers IR_FLAG_NOTHROW, the call becomes DCE-eligible as desired. Fixes #50311.