[PATCH] D40594: [InstCombine] miscompile of __builtin_fmod

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added a comment.

In https://reviews.llvm.org/D40594#940571, @spatel wrote:

> Added a blurb to the LangRef with https://reviews.llvm.org/rL319437, so I 
> think we can abandon this patch.


Sounds good.


https://reviews.llvm.org/D40594



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D40594: [InstCombine] miscompile of __builtin_fmod

[Sanjay Patel via Phabricator via cfe-commits]

spatel added a comment.

Added a blurb to the LangRef with https://reviews.llvm.org/rL319437, so I think 
we can abandon this patch.


https://reviews.llvm.org/D40594



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D40594: [InstCombine] miscompile of __builtin_fmod

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

By many years of precedent, the "frem" instruction is supposed to match the C 
fmod(), as opposed to something else like the C99 remainder(); probably worth 
clarifying in LangRef.


https://reviews.llvm.org/D40594



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D40594: [InstCombine] miscompile of __builtin_fmod

[Sanjay Patel via Phabricator via cfe-commits]

spatel added a comment.

Side note: I think there is a different bug here in clang because from what I 
can tell, we convert the builtin or libcall to 'frem' even when errno could be 
set by the call. https://reviews.llvm.org/D40044 doesn't address this case 
because frem is an LLVM instruction rather than an LLVM intrinsic.


https://reviews.llvm.org/D40594



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D40594: [InstCombine] miscompile of __builtin_fmod

[Sanjay Patel via Phabricator via cfe-commits]

spatel added reviewers: efriedma, hfinkel.
spatel added a comment.

I don't know the history of the frem instruction in IR, and the description in 
http://llvm.org/docs/LangRef.html#frem-instruction is vague.

But based on the existing code, I think this is working as intended. If the 
instruction has the same semantics as the math library function 'fmod', then we 
do want to convert this to an IR instruction in clang.

Note that when I filed PR34870, I assumed the bug was in the IR optimizer in 
the instcombine pass, but if the IR instruction does not match the semantics of 
the math library function, then I'd be wrong. :)


https://reviews.llvm.org/D40594



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D40594: [InstCombine] miscompile of __builtin_fmod

[Dmitry Venikov via Phabricator via cfe-commits]

Quolyk created this revision.

Motivation: https://bugs.llvm.org/show_bug.cgi?id=34870
I'm totally not sure this is correct


https://reviews.llvm.org/D40594

Files:
  lib/CodeGen/CGBuiltin.cpp
  test/CodeGen/builtins.c


Index: test/CodeGen/builtins.c
===
--- test/CodeGen/builtins.c
+++ test/CodeGen/builtins.c
@@ -253,13 +253,16 @@
   volatile long double resld;
 
   resf = __builtin_fmodf(F,F);
-  // CHECK: frem float
+  // CHECK: call float @fmodf(float %
+  // CHECK-NOT: readnone
 
   resd = __builtin_fmod(D,D);
-  // CHECK: frem double
+  // CHECK: call double @fmod(double %
+  // CHECK-NOT: readnone
 
   resld = __builtin_fmodl(LD,LD);
-  // CHECK: frem x86_fp80
+  // CHECK: call x86_fp80 @fmodl(x86_fp80 %
+  // CHECK-NOT: readnone
 
   resf = __builtin_fabsf(F);
   resd = __builtin_fabs(D);
Index: lib/CodeGen/CGBuiltin.cpp
===
--- lib/CodeGen/CGBuiltin.cpp
+++ lib/CodeGen/CGBuiltin.cpp
@@ -899,14 +899,6 @@
   case Builtin::BI__builtin_fabsl: {
 return RValue::get(emitUnaryBuiltin(*this, E, Intrinsic::fabs));
   }
-  case Builtin::BI__builtin_fmod:
-  case Builtin::BI__builtin_fmodf:
-  case Builtin::BI__builtin_fmodl: {
-Value *Arg1 = EmitScalarExpr(E->getArg(0));
-Value *Arg2 = EmitScalarExpr(E->getArg(1));
-Value *Result = Builder.CreateFRem(Arg1, Arg2, "fmod");
-return RValue::get(Result);
-  }
   case Builtin::BI__builtin_copysign:
   case Builtin::BI__builtin_copysignf:
   case Builtin::BI__builtin_copysignl: {


Index: test/CodeGen/builtins.c
===
--- test/CodeGen/builtins.c
+++ test/CodeGen/builtins.c
@@ -253,13 +253,16 @@
   volatile long double resld;
 
   resf = __builtin_fmodf(F,F);
-  // CHECK: frem float
+  // CHECK: call float @fmodf(float %
+  // CHECK-NOT: readnone
 
   resd = __builtin_fmod(D,D);
-  // CHECK: frem double
+  // CHECK: call double @fmod(double %
+  // CHECK-NOT: readnone
 
   resld = __builtin_fmodl(LD,LD);
-  // CHECK: frem x86_fp80
+  // CHECK: call x86_fp80 @fmodl(x86_fp80 %
+  // CHECK-NOT: readnone
 
   resf = __builtin_fabsf(F);
   resd = __builtin_fabs(D);
Index: lib/CodeGen/CGBuiltin.cpp
===
--- lib/CodeGen/CGBuiltin.cpp
+++ lib/CodeGen/CGBuiltin.cpp
@@ -899,14 +899,6 @@
   case Builtin::BI__builtin_fabsl: {
 return RValue::get(emitUnaryBuiltin(*this, E, Intrinsic::fabs));
   }
-  case Builtin::BI__builtin_fmod:
-  case Builtin::BI__builtin_fmodf:
-  case Builtin::BI__builtin_fmodl: {
-Value *Arg1 = EmitScalarExpr(E->getArg(0));
-Value *Arg2 = EmitScalarExpr(E->getArg(1));
-Value *Result = Builder.CreateFRem(Arg1, Arg2, "fmod");
-return RValue::get(Result);
-  }
   case Builtin::BI__builtin_copysign:
   case Builtin::BI__builtin_copysignf:
   case Builtin::BI__builtin_copysignl: {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits