[PATCH] PR16258 - fadd with undef not folded

[Duncan Sands]

Hi Joey,

On 10/06/13 16:07, Joey Gouly wrote:
> Hi baldrick,
>
> This is a fix for PR16258. We do not currently fold fadd, fsub and fmul to undef.

thanks for working on this.  However is it correct?  For example, in
   fadd X, Y
if X is a NaN, is it possible, by varying Y, to get every possible bit
pattern for the result?  Alternatively, is there a bit pattern that
would cause the fadd to trap?  Same question for every other possible
value for X.  If for every X, one of the answers is "yes" then it is OK
to fold "fadd X, undef" to "undef", otherwise it is not OK.  Because folding
to undef means every possible bit pattern for the result can occur.  If it is
not OK, it might still be OK with fast-math since we can ignore NaN's and some
other oddities.

Another way to get undef is if there is some bit pattern for Y that the IEEE
standard doesn't give a meaning to, in which case we can imagine that the undef
was this bit pattern and then we can assign any value we like to the fadd.

If undef can't be achieved, it might still be possible to fold to a NaN or
some other special value.

Unfortunately, I don't know the answers to these questions.  Hopefully one of
the floating point experts will chip in.

Ciao, Duncan.

>
> http://llvm-reviews.chandlerc.com/D942
>
> Files:
>    lib/Analysis/InstructionSimplify.cpp
>    test/Transforms/InstCombine/select-crash.ll
>    test/Transforms/InstSimplify/undef.ll
>
> Index: lib/Analysis/InstructionSimplify.cpp
> ===================================================================
> --- lib/Analysis/InstructionSimplify.cpp
> +++ lib/Analysis/InstructionSimplify.cpp
> @@ -877,6 +877,10 @@
>       std::swap(Op0, Op1);
>     }
>
> +  // fadd X, undef ==> undef
> +  if (match(Op1, m_Undef()))
> +    return Op1;
> +
>     // fadd X, -0 ==> X
>     if (match(Op1, m_NegZero()))
>       return Op0;
> @@ -916,6 +920,10 @@
>       }
>     }
>
> +  // fsub X, undef ==> undef
> +  if (match(Op1, m_Undef()))
> +    return Op1;
> +
>     // fsub X, 0 ==> X
>     if (match(Op1, m_Zero()))
>       return Op0;
> @@ -956,6 +964,9 @@
>       // Canonicalize the constant to the RHS.
>       std::swap(Op0, Op1);
>    }
> +  // fmul X, undef ==> undef
> +  if (match(Op1, m_Undef()))
> +    return Op1;
>
>    // fmul X, 1.0 ==> X
>    if (match(Op1, m_FPOne()))
> Index: test/Transforms/InstCombine/select-crash.ll
> ===================================================================
> --- test/Transforms/InstCombine/select-crash.ll
> +++ test/Transforms/InstCombine/select-crash.ll
> @@ -4,7 +4,7 @@
>   define fastcc double @gimp_operation_color_balance_map(float %value, double %highlights) nounwind readnone inlinehint {
>   entry:
>   ; CHECK: gimp_operation_color_balance_map
> -; CHECK: fsub double -0.000000
> +; CHECK: ret double undef
>     %conv = fpext float %value to double
>     %div = fdiv double %conv, 1.600000e+01
>     %add = fadd double %div, 1.000000e+00
> Index: test/Transforms/InstSimplify/undef.ll
> ===================================================================
> --- test/Transforms/InstSimplify/undef.ll
> +++ test/Transforms/InstSimplify/undef.ll
> @@ -153,3 +153,24 @@
>     %r = call i64 (i64)* undef(i64 %a)
>     ret i64 %r
>   }
> +
> +; CHECK: @test19
> +; CHECK: ret double undef
> +define double @test19(double %x) {
> +  %tmp = fadd double %x, undef
> +  ret double %tmp
> +}
> +
> +; CHECK: @test20
> +; CHECK: ret double undef
> +define double @test20(double %x) {
> +  %tmp = fsub double %x, undef
> +  ret double %tmp
> +}
> +
> +; CHECK: @test21
> +; CHECK: ret double undef
> +define double @test21(double %x) {
> +  %tmp = fmul double %x, undef
> +  ret double %tmp
> +}
>