[InstCombine] miscompile of __builtin_fmod

[Quolyk]

Summary

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

[spatel]

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. :)

[spatel]

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. D40044 doesn't address this case because frem is an LLVM instruction rather than an LLVM intrinsic.

[efriedma]

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.

[spatel]

Added a blurb to the LangRef with rL319437, so I think we can abandon this patch.

[hfinkel]

In D40594#940571, @spatel wrote:

Added a blurb to the LangRef with rL319437, so I think we can abandon this patch.

Sounds good.

lib/CodeGen/CGBuiltin.cpp

case Builtin::BI__builtin_llabs: {
 
     return RValue::get(Result);
   }
   case Builtin::BI__builtin_fabs:
   case Builtin::BI__builtin_fabsf:
   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: {
     return RValue::get(emitBinaryBuiltin(*this, E, Intrinsic::copysign));
   }
   case Builtin::BI__builtin_ceil:
   case Builtin::BI__builtin_ceilf:
   case Builtin::BI__builtin_ceill: {

test/CodeGen/builtins.c

 
 // CHECK-LABEL: define void @test_float_builtin_ops
 void test_float_builtin_ops(float F, double D, long double LD) {
   volatile float resf;
   volatile double resd;
   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);
   resld = __builtin_fabsl(LD);
   // CHECK: call float @llvm.fabs.f32(float
   // CHECK: call double @llvm.fabs.f64(double
   // CHECK: call x86_fp80 @llvm.fabs.f80(x86_fp80