[PATCH] D116952: [ConstantFolding] Respect denormal handling mode attributes when folding instructions

[Nuno Lopes via Phabricator via llvm-commits]

nlopes added a comment.

In D116952#3603700 <https://reviews.llvm.org/D116952#3603700>, @shchenz wrote:

> Hi @nlopes, thanks for providing the useful info. However I am still not very clear about how to deal with our internal failure after this patch.
>
>   define i1 @foo() denormal-fp-math=positive-zero,positive-zero {
>     %_add = fadd double 0.000000, 0.000000, exceptions=ignore
>     %_res = fcmp une double %_add, 0.000000
>     ret i1 %_res
>   }
>   =>
>   define i1 @foo() noread nowrite nofree willreturn denormal-fp-math=positive-zero,positive-zero {
>     ret i1 1
>   }
>
> The alive result is very confusing. I don't understand why `0.000000 + 0.000000` != `0x000000` when  `denormal-fp-math=positive-zero`, could you help to explain?

True, the output isn't great (floats are truncated, hence the 0.000000, which is not what's underneath). But what matters is the final result.

> I know you said online Alive2 is outdated, but seems the online Alive2 gets opposite result for the above 0.000000 case, it verifies `ret i1 0` as the valid transformation. https://alive2.llvm.org/ce/z/PjhR3U

The online version if Alive2 doesn't implement the denormal-fp-math attribute.

> There is a C case too:
>
>   int main(void)
>   {
>     double a = 1.264810e-321;
>     double b = 3.789480e-321;
>   
>     return (a + b != 5.054290e-321);
>   }
>
> `clang 1.c -Ofast -fdenormal-fp-math=positive-zero` without this patch, it gets 0 and with this patch, it gets 1. Our internal test expects 0 here.

Look at the generated assembly with -O0 without and without -fdenormal-fp-math. There's no difference. So it seems that this flag doesn't guarantee anything (it's best effort) or it's not fully implemented yet.
Nevertheless, your internal test is wrong. Check the math here: https://en.wikipedia.org/wiki/Double-precision_floating-point_format#Exponent_encoding


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116952/new/

https://reviews.llvm.org/D116952