Check if both branches of an Cmm if have the same target.
ClosedPublic

Authored by AndreasK on May 26 2018, 1:41 PM.

Details

Summary

This for some reason or the other and makes it into the final
binary. I've added the check to ContFlowOpt as that seems
like a logical place for this.

In a regular nofib run there were 30 occurences of this pattern.

Test Plan

ci

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.
AndreasK created this revision.May 26 2018, 1:41 PM
dfeuer requested changes to this revision.May 27 2018, 7:12 PM
dfeuer added a subscriber: dfeuer.

Something isn't working. Also, it would be nice to have a test demonstrating that this sometimes does something useful.

This revision now requires changes to proceed.May 27 2018, 7:12 PM

Something isn't working.

Could you be more specific?

Also, it would be nice to have a test demonstrating that this sometimes does something useful.

Fair point.

AndreasK updated this revision to Diff 16553.May 28 2018, 4:45 AM
  • Add test case
simonmar accepted this revision.May 28 2018, 5:41 AM

Nice!

bgamari accepted this revision.May 28 2018, 11:35 AM

Seems quite reasonable to me.

AndreasK updated this revision to Diff 16564.May 28 2018, 4:08 PM

Keep CFG around in NatM state and update it during codegen.

AndreasK updated this revision to Diff 16565.May 28 2018, 4:19 PM
  • Resubmmit the patch. Overwrote this one with another branch by mistake.
tdammers requested changes to this revision.May 30 2018, 10:49 AM
tdammers added a subscriber: tdammers.

Might be confusing for an unsuspecting reader to read hints about swapping the branches of a conditional, and then seeing code that also reduces conditionals down to just the plain branches.

compiler/cmm/CmmContFlowOpt.hs
289

Hmm, but now the name swapcond is no longer completely appropriate, is it?

This revision now requires changes to proceed.May 30 2018, 10:49 AM
AndreasK planned changes to this revision.May 30 2018, 2:18 PM
AndreasK added inline comments.
compiler/cmm/CmmContFlowOpt.hs
289

Good point. I will update this tomorrow.

AndreasK updated this revision to Diff 16598.May 30 2018, 5:21 PM
  • Update expression name
AndreasK marked 2 inline comments as done.May 30 2018, 5:22 PM

Your test is failing on OS X; seems to be a difference in how the wc implementations format their output - the Linux one left-aligns the 0, while the OS X one pads it with spaces. A not very elegant, but effective, solution would be to throw a quick sed job into the pipeling, something like | sed -e 's/^\s*//' maybe?

A better solution, actually, would be to just check grep's exit code:

EXIT STATUS

Normally  the  exit  status is 0 if a line is selected, 1 if no lines were selected,
and 2 if an error occurred.  However, if the -q or --quiet or --silent is used and a
line is selected, the exit status is 0 even if an error occurred.

I'm not 100% sure, but I believe this behavior is standard across grep implementations.

AndreasK updated this revision to Diff 16606.May 31 2018, 5:52 AM
  • Rely on grep exit status instead of wc in test.
bgamari accepted this revision.Jun 2 2018, 5:54 PM

Awesome, looks good.

This revision was not accepted when it landed; it landed in state Needs Review.Jun 7 2018, 7:52 PM
This revision was automatically updated to reflect the committed changes.