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

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

In D63405#1546424 <https://reviews.llvm.org/D63405#1546424>, @arsenm wrote:

> 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


The original code is wrong then (assuming we're using FMF on a value with the same reasoning that we use in IR/DAG). I'm not seeing how 'union' of flags is the correct fix though.


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

https://reviews.llvm.org/D63405