[PATCH] D63405: GlobalISel: Don't lose fneg flags when lowering to fsub

Mon Jun 17 09:45:14 PDT 2019

arsenm added a comment.

In D63405#1546410 <https://reviews.llvm.org/D63405#1546410>, @spatel wrote:

> In D63405#1546348 <https://reviews.llvm.org/D63405#1546348>, @arsenm wrote:
>
> > In D63405#1546342 <https://reviews.llvm.org/D63405#1546342>, @spatel wrote:
> >
> > > I'm not familiar with how flags are being used in this layer, but union of flags doesn't match the way we deal with FMF in IR. If the fneg is strict (disregard the irrelevant 'arcp' in that last test), then it's invalid to assume that it inherits the 'nsz' property from its operand.
> >
> >
> > I would expect the lowering here to just preserve the flag on the original instruction, and not propagate the source (as was done here previously). The corresponding expansion in the DAG has a TODO to preserve the flags, so there isn't precedent for this
>
>
> Let me know if I'm not seeing this correctly (haven't looked at MIR very much - it might help if the test was committed with the baseline output first, so we just have a diff in this patch). 
>  Did we start with 'fneg arcp X' and end up with 'fsub arcp nsz -0.0, X'? Preserving (rather than unioning) the flag should end up with only 'fsub arcp -0.0, X'?

The pre-patch code would fold fneg (fadd flags x, y) -> fsub flags -0.0, (fadd flags x, y), and entirely ignore any flags on fneg. I'm not entirely sure this is correct on its own

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

https://reviews.llvm.org/D63405