Page MenuHomePhabricator

fix an invisible bug when combining repeated FP divisors
ClosedPublic

Authored by spatel on May 20 2015, 1:44 PM.

Details

Summary

This patch fixes bugs that were exposed by r237046 ( http://reviews.llvm.org/rL237046 ):

  1. When replacing a division node, it's not enough to RAUW. We should call CombineTo() to delete dead nodes and combine again.
  2. Because we are changing the DAG, we can't return an empty SDValue after the transform. As the code comments say:

    Visitation implementation - Implement dag node combining for different node types. The semantics are as follows: Return Value: SDValue.getNode() == 0 - No change was made SDValue.getNode() == N - N was replaced, is dead and has been handled. otherwise - N should be replaced by the returned Operand.

The new test case shows no difference with or without this patch, but it will crash if we re-apply r237046. If anyone sees a way to expose the bug without FMF, I'd be happy to modify the test case.

Diff Detail

Repository
rL LLVM

Event Timeline

spatel updated this revision to Diff 26173.May 20 2015, 1:44 PM
spatel retitled this revision from to fix an invisible bug when combining repeated FP divisors.
spatel updated this object.
spatel edited the test plan for this revision. (Show Details)
spatel added reviewers: nicholas, hfinkel, ab, andreadb.
spatel added a subscriber: Unknown Object (MLST).
hfinkel added inline comments.May 20 2015, 1:56 PM
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
8312 ↗(On Diff #26173)

So once we have FMF, you'll add them here?

8324 ↗(On Diff #26173)

I'm not sure this is the right solution. Instead, we may want to check for a pre-existing reciprocal node, and use it if it already exists instead of replacing it with, perhaps, looser FMFs.

spatel added inline comments.May 20 2015, 2:18 PM
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
8312 ↗(On Diff #26173)

Right - once we have FMF, this recip node should at least have 'arcp'. And that's a bug in the existing patch in D9708 ( http://reviews.llvm.org/D9708 ) because I forgot about that.

And I think any FMUL that we create below would also have FMF because it was created under an FMF precondition.

8324 ↗(On Diff #26173)

This is where things get even foggier for me. :)
To make this optimization, don't we need to check the FMF of each U node? Eg, if one of these users does not have 'arcp', it is not eligible for replacement. I did check this in the current draft in D9708.

Phab test: I sent replies to Hal's comments an hour ago, but don't see anything on the commits list.

Adding bogus inline replies in an attempt to get the real replies that I sent yesterday to the commits list.

lib/CodeGen/SelectionDAG/DAGCombiner.cpp
8312 ↗(On Diff #26173)

.

8324 ↗(On Diff #26173)

.

Ping.

If possible, I'd like to postpone the FMF question and just decide what the right solution for this bug is independently of FMF.
Ie, is it necessary to use CombineTo here rather than RAUW? And is it wrong to return an empty SDValue() after we changed the DAG?

It's too soon to ping again :)
but I should mention that FMF for the DAG made it back into trunk with:
http://reviews.llvm.org/D10403

The functionality is hidden behind an enable flag (disabled by default), so we can expose the bug that this patch attempts to solve by using:

$ llc -enable-fmf-dag foo.ll

If we can resolve this bug, we can move on to other bugs that FMF may have exposed.

hfinkel accepted this revision.Jul 8 2015, 9:52 PM
hfinkel edited edge metadata.

Ping.

If possible, I'd like to postpone the FMF question and just decide what the right solution for this bug is independently of FMF.

Okay, but please add a FIXME. Otheriwse, LGTM.

Ie, is it necessary to use CombineTo here rather than RAUW? And is it wrong to return an empty SDValue() after we changed the DAG?

This revision is now accepted and ready to land.Jul 8 2015, 9:52 PM
This revision was automatically updated to reflect the committed changes.

If possible, I'd like to postpone the FMF question and just decide what the right solution for this bug is independently of FMF.

Okay, but please add a FIXME. Otheriwse, LGTM.

Thanks, Hal! Added a FIXME and checked in at r241826.

Now to ping Nick on the other thread to see what else was broken by FMF. :)