Diff Detail
Event Timeline
Do we not need any fast math flags here?
https://llvm.org/docs/LangRef.html#id276
The ‘sitofp’ instruction interprets its operand as a signed integer quantity and converts it to the corresponding floating-point value. If the value cannot be exactly represented, it is rounded using the default rounding mode.
| test/Transforms/InstSimplify/round-intrinsics.ll | ||
|---|---|---|
| 4 | Please precommit test | |
Right, *rounded*.
#include <math.h>
#include <limits>
#include <cassert>
static bool test(int x) {
float f = x;
float i;
float fract = modff(f, &i);
return fract == 0.0;
}
int main() {
for(size_t i = 0; i <= std::numeric_limits<unsigned int>::max(); i++)
assert(test(int(i)));
return 0;
}agrees.
Might also want to fold
%conv.i = sitofp i32 %conv1 to float %call.i = call float @modff(float %conv.i, float* nonnull %i.i) #4 %cmp.i = fcmp oeq float %call.i, 0.000000e+00 => %cmp.i = true
| lib/Analysis/InstructionSimplify.cpp | ||
|---|---|---|
| 4710–4711 | Matching uitofp is not included here for patch minimalism, or is there some functional difference that needs to be accounted for? | |
| test/Transforms/InstSimplify/round-intrinsics.ll | ||
|---|---|---|
| 43 | Missing tests for llvm.ceil? | |
I'm still a bit unsure about this. I could be simply confused though.
I agree that it is fine for those integral types when iN.SINT_MAX() <= float.MAX() && iN.SINT_MIN() >= float.MIN().
That means, we are good for up to i128 for float / i1024 for double or so.
But what about larger types? I don't think [su]itofp clamps?
"If the value cannot be exactly represented, it is rounded using the default rounding mode."
It's a maze of docs, but this is well-defined:
https://llvm.org/docs/LangRef.html#sitofp-to-instruction
"If the value cannot be exactly represented, it is rounded using the default rounding mode."
And then we have:
https://llvm.org/docs/LangRef.html#floatenv
"Results assume the round-to-nearest rounding mode."
So then we refer back to IEEE754 (section 7.4 Overflow):
"...if the destination format’s largest finite number is exceeded in magnitude by what would have been the rounded floating-point result (see 4) were the exponent range unbounded. The default result shall be determined by the rounding-direction attribute and the sign of the intermediate result as follows:
a) roundTiesToEven and roundTiesToAway carry all overflows to ∞ with the sign of the intermediate result."
So if the FP result is infinity, we then check the behavior of the LLVM intrinsics, and they all say, for example:
https://llvm.org/docs/LangRef.html#llvm-floor-intrinsic
"This function returns the same values as the libm floor functions would, and handles error conditions in the same way."
I'm not sure where or if there are official docs for libm, but:
https://en.cppreference.com/w/cpp/numeric/math/floor
"If arg is ±∞, it is returned, unmodified"
So that means the simplification is safe...obvious, right? :)
Note: There's a proposal to add intrinsic siblings for the other direction (fp --> int...which can produce poison) here:
D54749
I'm not sure where or if there are official docs for libm
The official definitions of all the libm floating-point functions for IEEE754 floating-point are in the C standard, Annex F.
As far as I can tell, your reasoning is right. sitofp/uitofp must produce either an integral finite result, or +inf/-inf. If sitofp/uitofp produces an integral finite result, all these rounding functions do nothing. And if sitofp produces an infinity, all these rounding functions also do nothing, as explicitly stated in Annex F.
And if sitofp produces an infinity, all these rounding functions also do nothing, as explicitly stated in Annex F.
That is the bit i was missing, thank you.
Looks good to me then.
| test/Transforms/InstSimplify/round-intrinsics.ll | ||
|---|---|---|
| 43 | (please precommit) | |
LGTM - see inline comment (would be good to include something like that in the commit message too, so people not following this review see the explanation that we discussed here).
| lib/Analysis/InstructionSimplify.cpp | ||
|---|---|---|
| 4710–4711 | It would be nice to have a comment here summarizing why this is legal, so something like // Converting from int always results in a finite integral number or // infinity. For either of those inputs, these rounding functions // always return the same value, so the rounding can be eliminated. | |
Matching uitofp is not included here for patch minimalism, or is there some functional difference that needs to be accounted for?