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
[InstcombineCompares] generates incorrect code for double comparision #23101
Comments
Doesn't the 'uitofp' conversion imply that we know that %conv is >= 0? I think this transformation is correct. |
I also think this looks correct. How did you run into this? |
My apologies.. looks like the code generated is correct. ran into a problem with this patch when an application stopped working and the corresponding source code is as follow.
somehow above comparision code is transformed to following after instruction combining after this patch. br i1 true, label %if.then, label %if.end, as noted, there is no comparision check lpb1 > 0.0 and delete of lbp is being called when lpb1 is 0. will try to get simplified testcase for above. |
The test input c++ program is attached. as seen, the input is the pointer bp is initialized to value "-1" outside the function and the array starts from index 1. the explicit conversion to double in the statement
is removed in the llvm patch.this changes the comparison to unsigned comparision which removes the check for lbp1 > 0.0. This will make the delete to be called for 0th element, which is not initialized. from my perspective, the source code seem to be OK. let me know otherwise. |
The optimization seems correct because we are converting an unsigned value to a double. The fact that the underlying bit-pattern is -1 is not really relevant. |
Right, when you do the conversion, you end up with UINT_MAX, not -1. That's always bigger than 0. |
In the case I mentioned, bp is -1 and lbp is 0
Agree that lbp1 can not be negative, but it can be 0 and the condition check should not have been removed. |
According to your original report, the condition hasn't been removed: The icmp eq i64 0 is still there. |
Yes. Apologies for that, as it is incorrect test. Please look into comment #4 and the attached testcase. |
According to the semantics of getelementptr inbounds, lbp is poison if bp was INT_MAX. Alive2 shows that the transformation is correct..! :) https://alive2.llvm.org/ce/z/AGEgCL |
Closing as per above comments, there is no bug in LLVM here. |
Extended Description
for the attached testcase with following IR:
%i = ptrtoint i64* %add.ptr to i64
%conv = uitofp i64 %i to double
%cmp = fcmp ogt double %conv, 0.000000e+00
br i1 %cmp, label %if.then, label %if.else
opt -O2 generates following output
%i = ptrtoint i64* %add.ptr to i64
%cmp = icmp eq i64 %i, 0
br i1 %cmp, label %if.else, label %if.then
In the input test we had fcmp ogt, which got transformed into icmp eq which doesnt seem to be correct. In the test input we are checking add.ptr > 0.0 whereas in the transformed code it is checked if %add.ptr == 0
The issue presents in trunk and is result of following checkin
commit d883ca0
Author: Matt Arsenault Matthew.Arsenault@amd.com
Date: Tue Jan 6 15:50:59 2015 +0000
The text was updated successfully, but these errors were encountered: