[MergeICmps] Enable the MergeICmps Pass by default.

[courbet]

Summary

Now that PR33325 is fixed, this should always improve the generated code.

[spatel]

Can you create at least one llc test each for x86 and PPC, so we see the result of merging + expansion together? This could just be copying existing tests from test/Transforms/MergeICmps?

[courbet]

Add llc test for X86 and PPC

[courbet]

In D42793#995427, @spatel wrote:

Can you create at least one llc test each for x86 and PPC, so we see the result of merging + expansion together? This could just be copying existing tests from test/Transforms/MergeICmps?

Done. Can you double check the PPC test ? It does make sense to me, but I'm not an expert.

[spatel]

In D42793#996098, @courbet wrote:

In D42793#995427, @spatel wrote:

Can you create at least one llc test each for x86 and PPC, so we see the result of merging + expansion together? This could just be copying existing tests from test/Transforms/MergeICmps?

Done. Can you double check the PPC test ? It does make sense to me, but I'm not an expert.

Thanks. This test looks fine to me as PPC given the 'align 4' on the loads. I recommend checking these tests in now, so we just see the improvements from enabling MergeICmps when this patch is committed.

For reference, the current PPC codegen is something like this:

# %bb.0:                                # %entry
	lwz 5, 0(3)
	lwz 6, 0(4)
	cmplw	5, 6
	bne	0, .LBB0_2
# %bb.1:                                # %land.rhs.i
	lwz 3, 4(3)
	lwz 4, 4(4)
	cmpw	3, 4
	b .LBB0_3
.LBB0_2:
	crxor 2, 2, 2
.LBB0_3:                                # %opeq1.exit
	li 3, 0
	li 4, 1
	isel 3, 4, 3, 2
	blr

My concern is that I don't see consideration for misalignment when combining to larger ops. We generally don't care on x86, but it could be a problem for some PPC? This may be a non-issue if some other pass is expected to split misaligned memops later.

[courbet]

Rebase the tests to show codegen diffs.

[spatel]

In D42793#996217, @courbet wrote:

Rebase the tests to show codegen diffs.

Thanks!

Thinking about this a bit more, since MergeICmps and MemCmpExpansion use the same TLI hook, this shouldn't be a problem unless there's already a bug in PPC's enabling of that hook. Given that MemCmpExpansion was originally written with a PPC target in mind, there's not much risk of that.

So I think this patch is fine now, but let's give someone who's closer to current-day PPC metal a chance to reply.

[nemanjai]

The PPC code looks way nicer. The alignment is fine as well - unaligned scalar loads are cheap on modern PPC HW and the loads are in bounds. Of course, I have no comment on the X86 code as I don't know enough about the target.
LGTM.

[spatel]

The x86 side LGTM.

[nemanjai]

Just wanted to also mention that this passes a double bootstrap build/lit/lnt on PPC.

[courbet]

Thanks a lot for the review !

[mstorsjo]

FWIW, this broke compilation of Qt for x86 (at least on windows, and the reduced example also triggers for other x86 targets), see PR36557 for details. Do you have time to look at it sometime soon, or is it appropriate to revert this until that is sorted out?

lib/CodeGen/TargetPassConfig.cpp

 static cl::opt<bool> DisableCopyProp("disable-copyprop", cl::Hidden,
     cl::desc("Disable Copy Propagation pass"));
 static cl::opt<bool> DisablePartialLibcallInlining("disable-partial-libcall-inlining",
     cl::Hidden, cl::desc("Disable Partial Libcall Inlining"));
 static cl::opt<bool> EnableImplicitNullChecks(
     "enable-implicit-null-checks",
     cl::desc("Fold null checks into faulting memory operations"),
     cl::init(false), cl::Hidden);
-static cl::opt<bool>
-    EnableMergeICmps("enable-mergeicmps",
-                     cl::desc("Merge ICmp chains into a single memcmp"),
-                     cl::init(false), cl::Hidden);
+static cl::opt<bool> DisableMergeICmps("disable-mergeicmps",
+    cl::desc("Disable MergeICmps Pass"),
 static cl::opt<bool> PrintLSR("print-lsr-output", cl::Hidden,
     cl::desc("Print LLVM IR produced by the loop-reduce pass"));
 static cl::opt<bool> PrintISelInput("print-isel-input", cl::Hidden,
     cl::desc("Print LLVM IR input to isel pass"));
 static cl::opt<bool> PrintGCInfo("print-gc", cl::Hidden,
     cl::desc("Dump garbage collector data"));
 static cl::opt<bool> VerifyMachineCode("verify-machineinstrs", cl::Hidden,
     cl::desc("Verify generated machine code"),
if (PrintLSR)
       addPass(createPrintFunctionPass(dbgs(), "\n\n*** Code after LSR ***\n"));
   }
 
   if (getOptLevel() != CodeGenOpt::None) {
     // The MergeICmpsPass tries to create memcmp calls by grouping sequences of
     // loads and compares. ExpandMemCmpPass then tries to expand those calls
     // into optimally-sized loads and compares. The transforms are enabled by a
     // target lowering hook.
-    if (EnableMergeICmps)
+    if (!DisableMergeICmps)
       addPass(createMergeICmpsPass());
     addPass(createExpandMemCmpPass());
   }
 
   // Run GC lowering passes for builtin collectors
   // TODO: add a pass insertion point here
   addPass(createGCLoweringPass());
   addPass(createShadowStackGCLoweringPass());

test/CodeGen/Generic/llc-start-stop.ll

 ; Note: -verify-machineinstrs is used in order to make this test compatible with EXPENSIVE_CHECKS.
 ; RUN: llc < %s -debug-pass=Structure -stop-after=loop-reduce -verify-machineinstrs -o /dev/null 2>&1 \
 ; RUN:   | FileCheck %s -check-prefix=STOP-AFTER
 ; STOP-AFTER: -loop-reduce
 ; STOP-AFTER: Dominator Tree Construction
 ; STOP-AFTER: Loop Strength Reduction
 ; STOP-AFTER-NEXT: Verify generated machine code
 ; STOP-AFTER-NEXT: MIR Printing Pass
 
 ; RUN: llc < %s -debug-pass=Structure -stop-before=loop-reduce -o /dev/null 2>&1 | FileCheck %s -check-prefix=STOP-BEFORE
 ; STOP-BEFORE-NOT: -loop-reduce
 ; STOP-BEFORE: Dominator Tree Construction
 ; STOP-BEFORE-NOT: Loop Strength Reduction
 
 ; RUN: llc < %s -debug-pass=Structure -start-after=loop-reduce -o /dev/null 2>&1 | FileCheck %s -check-prefix=START-AFTER
-; START-AFTER: -machine-branch-prob -expandmemcmp
+; START-AFTER: -machine-branch-prob -mergeicmps
 ; START-AFTER: FunctionPass Manager
-; START-AFTER-NEXT: Expand memcmp() to load/stores
+; START-AFTER-NEXT: Merge contiguous icmps into a memcmp
 
 ; RUN: llc < %s -debug-pass=Structure -start-before=loop-reduce -o /dev/null 2>&1 | FileCheck %s -check-prefix=START-BEFORE
 ; START-BEFORE: -machine-branch-prob -domtree
 ; START-BEFORE: FunctionPass Manager
 ; START-BEFORE: Loop Strength Reduction
-; START-BEFORE-NEXT: Expand memcmp() to load/stores
+; START-BEFORE-NEXT: Merge contiguous icmps into a memcmp
 
 ; RUN: not llc < %s -start-before=nonexistent -o /dev/null 2>&1 | FileCheck %s -check-prefix=NONEXISTENT-START-BEFORE
 ; RUN: not llc < %s -stop-before=nonexistent -o /dev/null 2>&1 | FileCheck %s -check-prefix=NONEXISTENT-STOP-BEFORE
 ; RUN: not llc < %s -start-after=nonexistent -o /dev/null 2>&1 | FileCheck %s -check-prefix=NONEXISTENT-START-AFTER
 ; RUN: not llc < %s -stop-after=nonexistent -o /dev/null 2>&1 | FileCheck %s -check-prefix=NONEXISTENT-STOP-AFTER
 ; NONEXISTENT-START-BEFORE: "nonexistent" pass is not registered.
 ; NONEXISTENT-STOP-BEFORE: "nonexistent" pass is not registered.
 ; NONEXISTENT-START-AFTER: "nonexistent" pass is not registered.
 ; NONEXISTENT-STOP-AFTER: "nonexistent" pass is not registered.
 
 ; RUN: not llc < %s -start-before=loop-reduce -start-after=loop-reduce -o /dev/null 2>&1 | FileCheck %s -check-prefix=DOUBLE-START
 ; RUN: not llc < %s -stop-before=loop-reduce -stop-after=loop-reduce -o /dev/null 2>&1 | FileCheck %s -check-prefix=DOUBLE-STOP
 ; DOUBLE-START: start-before and start-after specified!
 ; DOUBLE-STOP: stop-before and stop-after specified!