Page MenuHomePhabricator

Utilize new SDNode flag functionality to expand current support for fmul
ClosedPublic

Authored by mcberg2017 on Jun 7 2018, 3:06 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

mcberg2017 created this revision.Jun 7 2018, 3:06 PM
spatel added inline comments.Jun 8 2018, 11:07 AM
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
10494 ↗(On Diff #150413)

No tests? Please rebase after rL334308.

Updated as requested with some additional coverage.

spatel accepted this revision.Jun 8 2018, 11:56 AM

LGTM.

This revision is now accepted and ready to land.Jun 8 2018, 11:56 AM
spatel added inline comments.Jun 8 2018, 11:56 AM
test/CodeGen/X86/fp-fold.ll
104 ↗(On Diff #150549)

Can remove the TODO now.

spatel requested changes to this revision.Jun 8 2018, 12:00 PM
spatel added inline comments.
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
724–726 ↗(On Diff #150549)

Hang on - I missed this change. Why does this require nsz?

This revision now requires changes to proceed.Jun 8 2018, 12:00 PM
mcberg2017 updated this revision to Diff 150554.EditedJun 8 2018, 12:26 PM

Good catch, it should be the same as fdiv, nnan for that case.

Good catch, it should be the save as fdiv, nnan for that case.

Hmm...how does NaN change anything?

Instcombine moves fneg without any FMF:

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

So Options.HonorSignDependentRoundingFPMath is sufficient to guard fmul negate and we can drop the unsafe context, is that only true for multiply? What about divide?

So Options.HonorSignDependentRoundingFPMath is sufficient to guard fmul negate and we can drop the unsafe context, is that only true for multiply? What about divide?

I've never looked at HonorSignDependentRoundingFPMath:

/// If this is enabled (set to true), the code generator must
/// assume that the rounding mode may dynamically change.

That would seem to preclude a bunch of transforms, but I don't know why that's guarding this particular transform.

We need to split this diff into its own patch and find the existing tests and/or add new tests, so we know what's going on here.

fmul and fdiv should be treated the same with regard to fneg transforms (although instcombine probably gets this wrong).

Removed negation context...

fmul updates...

spatel accepted this revision.Jun 12 2018, 8:44 AM

LGTM.

test/CodeGen/X86/fp-fold.ll
104 ↗(On Diff #150549)

Stale TODO still here.

This revision is now accepted and ready to land.Jun 12 2018, 8:44 AM
This revision was automatically updated to reflect the committed changes.