This is an archive of the discontinued LLVM Phabricator instance.

easing the constraint for isNegatibleForFree and GetNegatedExpression
ClosedPublic

Authored by mcberg2017 on Jun 11 2018, 3:46 PM.

Details

Summary

Here we relax the old constraint which utilized unsafe with the TargetOption flag HonorSignDependentRoundingFPMathOption, with the assertion that unsafe is no longer needed or never was required for correctness on FDIV/FMUL.

Diff Detail

Event Timeline

mcberg2017 created this revision.Jun 11 2018, 3:46 PM
mcberg2017 edited the summary of this revision. (Show Details)Jun 11 2018, 3:47 PM

Tests which depend upon HonorSignDependentRoundingFPMathOption for FMUL/FDIV:

test/CodeGen/AMDGPU/fmuladd.f16.ll
test/CodeGen/AMDGPU/fmuladd.f32.ll
test/CodeGen/AMDGPU/mad-mix.ll
test/CodeGen/AMDGPU/v_mac.ll
test/CodeGen/AMDGPU/v_mac_f16.ll
test/CodeGen/PowerPC/fneg.ll
test/CodeGen/X86/dag-fmf-cse.ll

I determined this by switching the value of the flag test to the opposite boolean value as in the code in the view.

There is also some discussion needed if we want to add a guard FMF flag to FMUL and FDIV either together or as separate and different flags within negation context.

I will be adding a test for the fadd path shortly.

With the test that hits negation context for both unsafe and fmf for fadd.

mcberg2017 added a comment.EditedJun 12 2018, 1:31 PM

Note, once D47909 is checked in I will add the cleanup code for getNode here which covers fadd, fmul and fsub.

spatel added inline comments.Jun 13 2018, 12:00 PM
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
731–732

Can you explain this diff? Is there an existing test for this? If not, please add 1.
IIUC, UnsafeFPMath overrode HonorSignDependenRoundingFPMathOption. Ie, if we had unsafe, then it didn't matter what honor-sign said, it was safe to do this transform. Now, we're removing that override?

mcberg2017 added inline comments.Jun 13 2018, 12:34 PM
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
731–732

There is a list above in the earlier comments about tests which are modified by HonorSignDependentRoundingFPMathOption's state so we are covered.

By and large you statement is correct.

My question then becomes: Is it sufficient for the case where fmf is controlling optimizations on IR, i.e. where Unsafe is false, that HonorSignDependentRoundingFPMathOption's state is sufficient to proceed?

Or should we add a fmf constraint for the case where Unsafe is false to allow more optimization? If so, nsz, other flags" Possibly different ones for fmul and fdiv.

If HonorSignDependentRoundingFPMathOption is sufficient, then this code review becomes only about fadd.

spatel added inline comments.
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
731–732

The listed tests are affected by removing this fold (inverting the logic), but that doesn't tell us what effect HonorSignDependentRoundingFPMathOption was supposed to have when combined with unsafe math. We need tests that toggle each of those params to make this change.

The only tests that I see that use that target option are:
test/CodeGen/ARM/build-attributes.ll
test/CodeGen/ARM/fnmul.ll

cc @efriedma in case he has any insight or historical perspective.

You can move the fadd diff into its own patch, so that doesn't have to stall while determining the fmul/fdiv part of this.

HonorSignDependentRounding is pretty clearly a failed experiment; I'd suggest killing it completely (in a separate patch).

Given Eli's feedback, I think we can keep fadd together here, I have removed the checks fully and added a test.

mcberg2017 added inline comments.Jun 13 2018, 4:34 PM
test/CodeGen/ARM/fnmul.ll
6

Now the baseline question becomes, do we want fmul/fdiv negation only under Unsafe or do we want to ungate it?
If just Unsafe, I modify this test case and split fadd out.

spatel added inline comments.Jun 14 2018, 7:33 AM
test/CodeGen/ARM/fnmul.ll
6

The fmul/fdiv negation fold is safe without any flags. In IR, we currently canonicalize the other way for fmul:

// Sink negation: -X * Y --> -(X * Y)

...but I'd still prefer that we split this change up from the fadd diff to reduce risk.

Removed fadd from this context

mcberg2017 edited the summary of this revision. (Show Details)Jun 14 2018, 10:05 AM
spatel accepted this revision.Jun 14 2018, 12:39 PM

LGTM.

This revision is now accepted and ready to land.Jun 14 2018, 12:39 PM
This revision was automatically updated to reflect the committed changes.