Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
[ConstantFold] Fold
log1p
andlog1pf
when the input parameter is a constant value. #112113New 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
[ConstantFold] Fold
log1p
andlog1pf
when the input parameter is a constant value. #112113Changes from all commits
b49c6c1
cbeb725
b071121
6e7496b
3c04e28
34a8746
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Test memory none special cases
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.
I have indeed considered handling
memory(none)
cases. My current plan is not to handle libc functions on a case-by-case basis, but to directly use the value from the libc function regardless of its parameter. Essentially, what I want is to pass theCallBase
into theconstantFold
function, which involves two scenarios. In Case 1, if there are no floating-point exceptions, everything is fine, and we return the folded value. In Case 2, if there is a floating-point exception, we return either nullptr or the exact value returned from the function (such as NaN, HUGE_VAL, etc.) depending on whether this call accesses memory. By doing this, we can eliminate the range check in the Constant Folding phase and avoid error handling based on each function. I look forward to hearing your thoughts on this initial plan.Handling
memory(none)
cases will impact all current folding operations. I believe it may break some tests and require additional testing. Therefore, my preference is to constant fold most common math functions as much as possible, and then address edge cases afterwards. Would this approach be acceptable?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.
I don't think the consideration of doesNotAccessMemory belongs down in ConstantFoldFP. It should produce the constant folded value regardless of the side effects. The legality of ignoring the side effects is context dependent
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.
Plus you can and should still add the tests for these cases here, even if they do not fold yet
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.
Done.
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.
But IIUC the current implementation of
ConstantFoldFP
already considers side effects? If errno is set or a floating-point exception occurs, it returns nullptr instead of the expected return value.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.
I would consider that a defect of implementation. The separation of concerns should yield the result. If we wanted, we could produce a side piece of information for raised errors. Whether or not you care if an error occurred depend on the callsite (i.e we can ignore any exceptions in default FP environment, and memory(none) callsites can ignore errno writes)
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.
I believe our ideas align. Even if the implementation of
ConstantFolfFP
directly returns the result from the underlying libc, we still need a wrapper to validate the libc function parameter. Utilizing fp exceptions and errno to determine the legitimacy of the parameter is convenient as it eliminates the need to write range checks for each individual function.