This is an archive of the discontinued LLVM Phabricator instance.

InstSimplify: Fold round intrinsics from sitofp
ClosedPublic

Authored by arsenm on Mar 29 2019, 5:54 PM.

Diff Detail

Event Timeline

arsenm created this revision.Mar 29 2019, 5:54 PM
arsenm updated this revision to Diff 192948.Mar 29 2019, 6:02 PM

Remove incorrect comment

arsenm updated this revision to Diff 192949.Mar 29 2019, 6:03 PM

Fix comment

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

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.

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
spatel added inline comments.Mar 31 2019, 7:51 AM
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?

arsenm updated this revision to Diff 193083.Apr 1 2019, 7:04 AM

Restore uitofp, rebase on top of added test

spatel added inline comments.Apr 1 2019, 7:07 AM
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?

arsenm added a comment.Apr 1 2019, 7:53 AM

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."

arsenm updated this revision to Diff 193089.Apr 1 2019, 7:53 AM

Fix missing ceil test

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?

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.

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.

That is the bit i was missing, thank you.

Looks good to me then.

test/Transforms/InstSimplify/round-intrinsics.ll
43

(please precommit)

spatel accepted this revision.Apr 2 2019, 5:50 AM

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.
This revision is now accepted and ready to land.Apr 2 2019, 5:50 AM
arsenm closed this revision.Apr 2 2019, 5:23 PM
arsenm marked an inline comment as done.

r357549