This is an archive of the discontinued LLVM Phabricator instance.

[BPI] Improve static heuristics for integer comparisons
ClosedPublic

Authored by xbolva00 on Aug 11 2020, 2:10 PM.

Diff Detail

Event Timeline

xbolva00 created this revision.Aug 11 2020, 2:10 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 11 2020, 2:10 PM
xbolva00 requested review of this revision.Aug 11 2020, 2:10 PM

Any perf data?

llvm/lib/Analysis/BranchProbabilityInfo.cpp
880

I think we better avoid extending the scope of 'isProb' variable. Please either introduce scope local variable or just use CI->isTrueWhenEqual directly in the 'if'.

xbolva00 added a comment.EditedAug 12 2020, 5:33 AM

Any perf data?

I observed nice improvement for zstd with silesia dataset.

Perf data: https://pastebin.com/N1JEzCMm

Decompression speed was improved quite significatly. ~ 940 MB/s -> 970 - 980 MB/s

xbolva00 updated this revision to Diff 285064.Aug 12 2020, 6:34 AM

Addressed review notes.

xbolva00 marked an inline comment as done.Aug 12 2020, 6:42 AM
This revision is now accepted and ready to land.Aug 12 2020, 9:23 PM
davidxl added inline comments.Aug 12 2020, 10:22 PM
llvm/lib/Analysis/BranchProbabilityInfo.cpp
876

Perhaps change ZH_TAKEN_WEIGHT to IH_TAKEN_WEIGHT. Similarly for ZH_NONTAKEN_WEIGHT

xbolva00 updated this revision to Diff 285314.Aug 13 2020, 3:42 AM
xbolva00 updated this revision to Diff 285316.Aug 13 2020, 3:43 AM
xbolva00 marked an inline comment as done.
This revision was automatically updated to reflect the committed changes.

How your last commit differs from the approved one? Why you have so many additional changes in the tests?

How your last commit differs from the approved one? Why you have so many additional changes in the tests?

Patch affects many codegen tests so I updated them mostly with updater scripts.

How your last commit differs from the approved one? Why you have so many additional changes in the tests?

Patch affects many codegen tests so I updated them mostly with updater scripts.

The question is why changes for these tests were not part of original review? Did you just missed to upload them or changes appeared later?

Missed. Some files were adjusted to be regenerated with update scripts, so some changes are bigger than needed.

Some tests have non-trivial changes. Some tests are actually enhanced. In this case, it'd be nice if a separate review (or just a commit, depending on the nature of the updates) to improve the tests first. Then this review can show the differences. It would be easier for a reader to understand the improvement (and also makes reverts easier if this turns out to be insufficient)

Some tests have non-trivial changes. Some tests are actually enhanced. In this case, it'd be nice if a separate review (or just a commit, depending on the nature of the updates) to improve the tests first. Then this review can show the differences. It would be easier for a reader to understand the improvement (and also makes reverts easier if this turns out to be insufficient)

FWIW i've already previously provided such feedback at least in https://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20200810/819028.html,
before https://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20200810/819138.html happened, and i *think* before that too.
But it seems like the feedback isn't being heard. I'm not sure what the problem is.

Feedback arrived after things landed. Should I revert everything and split it to many patches and reland?

https://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20200810/819028.html

Strange (?). I did not receive this mail. Next time please comment on Phab.

I suggest the following steps to commit this patch:

  1. add an internal option to turn on the new heuristics with default to

false;

  1. commit the patch with minimal test changes;
  1. for each target affected, explicitly modify the tests with the option

turned on and ask the target owner to examine the changes

  1. once all tests are fixed (with option explicitly turned on), flip the

option on by default;

  1. remove the explicit option added in step 3).

I suggest the following steps to commit this patch:

  1. add an internal option to turn on the new heuristics with default to

false;

  1. commit the patch with minimal test changes;
  1. for each target affected, explicitly modify the tests with the option

turned on and ask the target owner to examine the changes

  1. once all tests are fixed (with option explicitly turned on), flip the

option on by default;

  1. remove the explicit option added in step 3).

Good idea, thanks!

Feedback arrived after things landed. Should I revert everything and split it to many patches and reland?

https://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20200810/819028.html

Strange (?). I did not receive this mail.

Hm, are you not subscribed to/looking through llvm-commits mails?

Next time please comment on Phab.

No. But I got mails from Eli or Tom.

Dňa 17. 8. 2020 o 21:06 užívateľ Roman Lebedev via Phabricator <reviews@reviews.llvm.org> napísal:

lebedev.ri added a comment.

In D85781#2222087 https://reviews.llvm.org/D85781#2222087, @xbolva00 wrote:

Feedback arrived after things landed. Should I revert everything and split it to many patches and reland?

https://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20200810/819028.html

Strange (?). I did not receive this mail.

Hm, are you not subscribed to/looking through llvm-commits mails?

Next time please comment on Phab.

Repository:
rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D85781/new/

https://reviews.llvm.org/D85781