[PATCH] D37686: [DAG] Consolidating Instruction->SDNode Flags propagation in one class for better code management.

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

In https://reviews.llvm.org/D37686#877951, @jbhateja wrote:

> In https://reviews.llvm.org/D37686#877864, @spatel wrote:
>
> > I've added some more FMF tests at https://reviews.llvm.org/rL313893 which I think this patch will miscompile. Please rebase/update.
> >
> > As I suggested before, this patch shouldn't try to enable multiple DAG combines with node-level FMF. It's not as straightforward as you might think.
> >
> > Pick exactly one combine if you want to show that this patch is working as intended. The llvm.muladd intrinsic test that you have here with a target that supports 'fma' (not plain x86) seems like a good choice to me. If we have a strict op in IR, it should produce an fma instruction. If we have a fast op in IR, it should produce the simpler fmul instruction?
>
>
> My understanding and code changes are based LLVM Ref Manual 's section about Fast-Math flags"  (http://llvm.org/docs/LangRef.html#fast-math-flags)
>
> Which say for FMF flag NaN  "Allow optimizations to assume the arguments and result are not NaN".
>
> Now in following case which has been added by you
>
>   %y = call float @llvm.sqrt.f32(float %x)
>   %z = fdiv fast float 1.0, %y
>   ret float %z
>   
>
> We dont have fast flag over intrinsic but DAGCombining for fdiv sees a fast flag and assume result (%z) and arguments (constant , %y) as not a Nan and goes ahead and generates a reciprocal sqrt. If you remove fast from fdiv and add it to intrinsic then FMF opt at fdiv will not kick in.
>
> Can you please let me know what you expected here.


I expect that the sqrt result is strict. Ie, it should use sqrtss if this is x86-64. We're not allowed to use rsqrtss and lose precision on that op.

That said, my memory of exactly how op-level FMF should work is fuzzy. If anyone else remembers or can link to threads where we've discussed this, please feel free to jump in. :)


https://reviews.llvm.org/D37686