[PATCH] D24422: Unsafe copysign xform in DAGCombiner

[Cameron McInally via llvm-commits]

cameron.mcinally added a comment.

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.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:9025
@@ -9024,2 +9024,3 @@
   // copysign(x, copysign(y,z)) -> copysign(x, z)
-  if (N1.getOpcode() == ISD::FCOPYSIGN)
+  if (DAG.getTarget().Options.UnsafeFPMath &&
+      N1.getOpcode() == ISD::FCOPYSIGN)
----------------
delena wrote:
> cameron.mcinally wrote:
> > delena wrote:
> > > You talked about "convert" between two "copysign". That's what you put in your test. Looks like "fpext" is handled 3 lines bellow.
> > 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.
> 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".
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...


https://reviews.llvm.org/D24422