This is an archive of the discontinued LLVM Phabricator instance.

[Machine Combiner] Valid use of OptSize
ClosedPublic

Authored by avt77 on Feb 27 2018, 2:33 AM.

Details

Summary

Currently we're able to select longer alternative code sequence if it has better sched numbers but we should always select the shortest code sequence if OptSize == true. This patch fixes the given issue.

Diff Detail

Event Timeline

avt77 created this revision.Feb 27 2018, 2:33 AM
avt77 added a comment.Feb 28 2018, 4:51 AM

test case?

Unfortunately, before D40602 (or D26855 which I'm going to update soon) we don't have any test case because we don't use Machine Combiner when alternative sequence of instructions is longer then the original one.
But D40602 has such test just now (see lines 420-423 in test/CodeGen/X86/schedule-x86-64-shld.ll): at the moment we can select longer code sequence even if we have optsize option for the function (see define void @lshift_mem_b_optsize(i64 %b) nounwind readnone optsize {).

avt77 added a comment.Mar 13 2018, 3:13 AM

Ping!
Gerolf, could you give me LGTM for this tiny patch?

LGTM with a couple of style comments - pity we can't get a test case

lib/CodeGen/MachineCombiner.cpp
413
return (NewSize < OldSize);  // Only substitute if new size < old size
594
} else if (!OptSize || (NewInstCount <= OldInstCount))) {
avt77 added a comment.Mar 16 2018, 9:03 AM

LGTM with a couple of style comments - pity we can't get a test case

Could you press "Accepted"?

This revision was not accepted when it landed; it landed in state Needs Review.Mar 16 2018, 9:08 AM
This revision was automatically updated to reflect the committed changes.