Be more selective in which conditionals we invert
ClosedPublic

Authored by simonmar on Feb 8 2018, 4:18 AM.

Details

Summary

See Note [improveConditional] for details.

Test Plan

validate

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
simonmar created this revision.Feb 8 2018, 4:18 AM
simonmar updated this revision to Diff 15408.Feb 8 2018, 4:20 AM

fix neLike

AndreasK requested changes to this revision.Feb 8 2018, 4:53 AM

I think the comment is much improved already but still slightly misleading.

compiler/cmm/CmmSink.hs
497–508

So we look for that specific case here.

This was never really true was it? We happily invert all conditionals that match the pattern if they make it this far.
Floating point or not.

It works because all other(non float) expressions should have been simplified by this point.
But we don't check for that so it's worth pointing that out in a comment.

This revision now requires changes to proceed.Feb 8 2018, 4:53 AM
simonmar added inline comments.Feb 8 2018, 7:07 AM
compiler/cmm/CmmSink.hs
497–508

I've made it more specific than it used to be - before it was inverting every !=, now it's only doing that if it can save a comparison. Currently I believe the only cases where that happens are the floating-point ones, but it doesn't do any harm to do it for any comparison. I'll clarify the comment.

simonmar updated this revision to Diff 15410.Feb 8 2018, 7:11 AM

Clarify comment

AndreasK accepted this revision.Feb 8 2018, 9:59 AM
This revision is now accepted and ready to land.Feb 8 2018, 9:59 AM
simonmar updated this revision to Diff 15701.Mar 8 2018, 5:51 AM

fix warning

This revision was automatically updated to reflect the committed changes.

Is there a ticket, or test case, for this?

Is there a ticket, or test case, for this?

It's a fixup to this previous change: https://phabricator.haskell.org/rGHC6d14c1485cb570cbd183bcdc0f858d9a6dc1eb31

No testcase I'm afraid (but there wasn't one for the original change either).