Unsafe copysign xform in DAGCombiner

[cameron.mcinally]

Summary

Migrating from the llvm-commits thread of the same name...

There's a copysign xform in the DAGCombiner that replaces safe FP code
with unsafe code. Attached is a patch to guard this xform.

The issue here (see test case) is the first copysign is removed before
the cvt is performed. If the input to the cvt is unordered, it will
raise an exception. The purpose of the first copysign is to create a
safe value (i.e. 1.0 or -1.0) to feed the cvt.

Also, I'm not the most versed in FileCheck. I'd appreciate any
critiques on the test case I've added.

[cameron.mcinally]

Add context to diff.

[delena]

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

9025

You talked about "convert" between two "copysign". That's what you put in your test. Looks like "fpext" is handled 3 lines bellow.

test/CodeGen/X86/sse-fcopysign.ll

161

you can use utility to generate all "CHECKS" utils/update_llc_test_checks.py

[cameron.mcinally]

Yes, good catch. So we begin with:

t8: f64 = fcopysign t4, t7

t4: f64,ch = CopyFromReg t0, Register:f64 %vreg1
  t0: ch = EntryToken
t7: f64 = fp_extend t6
  t6: f32 = fcopysign ConstantFP:f32<1.000000e+00>, t2
    t2: f32,ch = CopyFromReg t0, Register:f32 %vreg0

And the first pass of this function removes the *explicit* fpext:

t13: f64 = fcopysign t4, t6

t4: f64,ch = CopyFromReg t0, Register:f64 %vreg1
  t0: ch = EntryToken
t6: f32 = fcopysign ConstantFP:f32<1.000000e+00>, t2
  t2: f32,ch = CopyFromReg t0, Register:f32 %vreg0

That's misleading since there's still an *implicit* fpext there, from f32->f64 on t6.

I'm not versed enough in SDNodes to say whether removing the explicit fpext is okay or not. But the first copysign is necessary to sanitize the input before hitting the vcvt instruction, even if it's implicitly generated.

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

9025

Yes, good catch. So we begin with:

t8: f64 = fcopysign t4, t7

t4: f64,ch = CopyFromReg t0, Register:f64 %vreg1
  t0: ch = EntryToken
t7: f64 = fp_extend t6
  t6: f32 = fcopysign ConstantFP:f32<1.000000e+00>, t2
    t2: f32,ch = CopyFromReg t0, Register:f32 %vreg0

And the first pass of this function removes the *explicit* fpext:

t13: f64 = fcopysign t4, t6

t4: f64,ch = CopyFromReg t0, Register:f64 %vreg1
  t0: ch = EntryToken
t6: f32 = fcopysign ConstantFP:f32<1.000000e+00>, t2
  t2: f32,ch = CopyFromReg t0, Register:f32 %vreg0

That's misleading since there's still an *implicit* fpext there, from f32->f64 on t6.

I'm not versed enough in SDNodes to say whether removing the explicit fpext is okay or not. But the first copysign is necessary to sanitize the input before hitting the vcvt instruction, even if it's implicitly generated.

[cameron.mcinally]

Eli mentioned that it's not worth pursuing this until the FP Env and RM work is merged.

I'm not sure where this deficiency should live for future consideration... here or Bugzilla or ???.

[delena]

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

9025

I assume that you should prevent removing fpext on the first pass.
Add more tests to see whether it's ok or not.
Anyway, removing common optimization without any type checking looks wrong to me.

You also can talk with the author of this code, find him with "svn blame".

[cameron.mcinally]

Comments inline...

I don't have a good fix for this case right now, so I'll have to keep thinking. But since Eli mentioned that this patch isn't worth pursuing right now, I'll probably put it on the back burner until the FE environment work is accepted.

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

9025

That seems like a reasonable change, but I think it also misses the mark...

We really want to prevent changes to:

fpext(copysign(a,b))

Not:

copysign(x, fpext(y))

In other words, we want to make sure the input to the fpext won't trap by keeping the copysign in place. I'm not sure how best to do it yet...

[cameron.mcinally]

This is no longer an issue after D55897. Abandoning...

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

if (N0.getOpcode() == ISD::FABS || N0.getOpcode() == ISD::FNEG ||
     return DAG.getNode(ISD::FCOPYSIGN, SDLoc(N), VT,
                        N0.getOperand(0), N1);
 
   // copysign(x, abs(y)) -> abs(x)
   if (N1.getOpcode() == ISD::FABS)
     return DAG.getNode(ISD::FABS, SDLoc(N), VT, N0);
 
   // copysign(x, copysign(y,z)) -> copysign(x, z)
-  if (N1.getOpcode() == ISD::FCOPYSIGN)
+  if (DAG.getTarget().Options.UnsafeFPMath &&
+      N1.getOpcode() == ISD::FCOPYSIGN)
     return DAG.getNode(ISD::FCOPYSIGN, SDLoc(N), VT,
                        N0, N1.getOperand(1));
 
   // copysign(x, fp_extend(y)) -> copysign(x, y)
   // copysign(x, fp_round(y)) -> copysign(x, y)
   if (CanCombineFCOPYSIGN_EXTEND_ROUND(N))
     return DAG.getNode(ISD::FCOPYSIGN, SDLoc(N), VT,
                        N0, N1.getOperand(0));

test/CodeGen/X86/sse-fcopysign.ll

; X64-NEXT:    retq
   %tmp1 = fadd float -1.0, -1.0
   %tmp2 = fpext float %tmp1 to double
   %tmp = tail call double @llvm.copysign.f64( double 0.0, double %tmp2 )
   ret double %tmp
 }
 
 declare float     @llvm.copysign.f32(float  %Mag, float  %Sgn)
 declare double    @llvm.copysign.f64(double %Mag, double %Sgn)
+
+;
+; Unsafe transformations
+;
+
+define double @safe(float %a, double %b) "unsafe-fp-math"="false" {
+; CHECK-LABEL: safe:
+; CHECK:         andps
+; CHECK-NEXT:    orps
+; CHECK:         cvtss2sd
+
+  %r1 = call float @llvm.copysign.f32(float 1.000000e+00, float %a)
+  %r2 = fpext float %r1 to double
+  %r3 = call double @llvm.copysign.f64(double %b, double %r2)
+  ret double %r3
+}