This is an archive of the discontinued LLVM Phabricator instance.

Fix Reassociate handling of constant in presence of undef float
ClosedPublic

Authored by mehdi_amini on Jan 15 2015, 12:25 AM.

Details

Reviewers
majnemer
Summary

Just implemented the patch you suggested along with the test case.

Diff Detail

Event Timeline

mehdi_amini retitled this revision from to Fix Reassociate handling of constant in presence of undef float .
mehdi_amini updated this object.
mehdi_amini edited the test plan for this revision. (Show Details)
mehdi_amini added a reviewer: majnemer.
mehdi_amini set the repository for this revision to rL LLVM.
mehdi_amini added a subscriber: Unknown Object (MLST).
spatel added a subscriber: spatel.Jan 15 2015, 8:45 AM
spatel added inline comments.
test/Transforms/Reassociate/undef.ll
4 ↗(On Diff #18216)

Please use FileCheck for the expected output rather than just assuming a lack of crash is correct.

Nit: "The Reassociate pass used to crash on these examples."

mehdi_amini added inline comments.Jan 15 2015, 9:53 AM
test/Transforms/Reassociate/undef.ll
4 ↗(On Diff #18216)

Why should I add a Filecheck for a crash? I looked at what was done by other crash test in the test suite.
What should I check exactly?

Thanks!

spatel added inline comments.Jan 15 2015, 10:15 AM
test/Transforms/Reassociate/undef.ll
4 ↗(On Diff #18216)

It's just improved testing. If you're going to add a test (which of course you have to), then you might as well make that test check for correctness rather than absence of incorrectness.

I realize that there are existing tests that violate this principle. They can and should be improved when possible.

In this case, you want something like (I didn't actually run this):
CHECK-LABEL: @undef1(
CHECK-NEXT: wrapper_entry:
CHECK-NEXT: ret float fadd

CHECK-LABEL: @undef2(
CHECK-NEXT: wrapper_entry:
CHECK-NEXT: unreachable

mehdi_amini retitled this revision from Fix Reassociate handling of constant in presence of undef float to Fix Reassociate handling of constant in presence of undef float.
mehdi_amini updated this object.

Update taking into account the review

(even though I'm not convince that sometime you *can* test something. Here for the first test the result is

ret float fadd (float undef, float fadd (float undef, float fadd (float fsub (float -0.000000e+00, float undef), float fsub (float -0.000000e+00, float undef))))

if someone improves Reassociate to fold undef that is equally fine in my opinion and we are just adding some maintenance burden/noise to update test.)

majnemer accepted this revision.Jan 15 2015, 7:54 PM
majnemer edited edge metadata.

LGTM

This revision is now accepted and ready to land.Jan 15 2015, 7:54 PM
In D6993#109628, @joker.eph wrote:

(even though I'm not convince that sometime you *can* test something. Here for the first test the result is

ret float fadd (float undef, float fadd (float undef, float fadd (float fsub (float -0.000000e+00, float undef), float fsub (float -0.000000e+00, float undef))))

if someone improves Reassociate to fold undef that is equally fine in my opinion and we are just adding some maintenance burden/noise to update test.)

That's true, but:

  1. You don't have to fully specify each line of IR in your CHECKs.
  2. You didn't create the minimal test case to check your patch. I haven't looked into how this crashes, but this is certainly a smaller test case that still crashes: define float @undef1(float %x) { %y = fsub fast float undef, %x %z = fsub fast float undef, %y ret float %z }