This is an archive of the discontinued LLVM Phabricator instance.

GlobalISel: Don't lose fneg flags when lowering to fsub
ClosedPublic

Authored by arsenm on Jun 17 2019, 4:32 AM.

Details

Summary

This was ignoring the flag on fneg, and using the source instruction's
flags which wasn't obviously correct looking to me. I think this is
OK, but should also keep any flags present on the original fneg.

Also fixes tests missing from r358702.

Diff Detail

Event Timeline

arsenm created this revision.Jun 17 2019, 4:32 AM
mcberg2017 accepted this revision.Jun 17 2019, 8:25 AM

Seems reasonable to me.

This revision is now accepted and ready to land.Jun 17 2019, 8:25 AM

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'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

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'?

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

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.

arsenm updated this revision to Diff 205108.Jun 17 2019, 10:11 AM

Just preserve the original flags

Ok, with the constraint like in the DAG:

as Unsafe or nsz on the Op

 SDAG is preserving the Op flags or in this case the MI flags in place of SrcMI.

IIRC, isn't preserving the original flags the outcome we want here? If so, I think the updated patch is fine.

@mcberg2017 is that correct?

... ah wait he already got to it while I was typing, never mind. :)

Should we not also guard GlobslIsel translation in this case and avoid if not met?

SDAG fails isNegatibleForFree if that constraint is not met and we do not translate.

Ok, with the constraint like in the DAG:

as Unsafe or nsz on the Op

 SDAG is preserving the Op flags or in this case the MI flags in place of SrcMI.

I don't follow. I don't see why the source should matter. The DAG currently does not preserve the flags here

I am saying you have it right Matt for using MI flags, but we should guard folding based on Unsafe or nsz in the Flags.

I am saying you have it right Matt for using MI flags, but we should guard folding based on Unsafe or nsz in the Flags.

There's no folding going on here? This is just a 1-op to 1-op legalization

We do this as a combine earlier in target specific code under this constraint. Matt, perhaps you just missing that combine in GlobalIsel?

mcberg2017 added a comment.EditedJun 17 2019, 10:58 AM

For your target. But as a catch all for legalization we should do this.

For your target. But as a catch all for legalization we should do this.

AMDGPU doesn't use this legalization. I'm just trying to fix a correctness issue in the generic legalizer

So I agree, but I think there should be a target specific combine with guards for Unsafe and nsz and this legalization case (for those who do not have it).

volkan accepted this revision.Jun 17 2019, 11:17 AM

I think we should preserve the existing flags and be consistent with what we do in SDAGISel. LGTM.

spatel accepted this revision.Jun 17 2019, 11:19 AM

This matches my mental model for FMF propagation, so LGTM.

But there's a separate question that is raised here: why is it legal to convert fneg to fsub -0.0? That loosens the IEEE requirement when dealing with a NAN. I'd think this should be legalized by converting to integer and flipping the sign bit (xor).
ping @cameron.mcinally

But there's a separate question that is raised here: why is it legal to convert fneg to fsub -0.0? That loosens the IEEE requirement when dealing with a NAN. I'd think this should be legalized by converting to integer and flipping the sign bit (xor).
ping @cameron.mcinally

Sanjay is correct. It’s not safe to convert fneg->fsub without nnan.

Should this expansion just be ripped out then? This is also broken in SelectionDAG. I don't like the idea of a legalization that relies on checking the flags, and this could be an optimization fold somewhere else

arsenm closed this revision.Jun 17 2019, 4:45 PM

r363637