[MachineSinking, RFC] Sink insert_subreg, subreg_to_reg, and reg_sequence instructions

[wmi]

Summary

In https://llvm.org/bugs/show_bug.cgi?id=28852, we saw a testcase for which machineSinking was blocked by subreg_to_reg so high cost operation cannot be moved to cold place.

The limitation about "insert_subreg, subreg_to_reg, and reg_sequence" was to make them close to the source and make them easier to coalesce. Will the coalesce indicated here happen at register coalescing or register allocation? Is the limitation still valid considering it was added at 2010 and register coalescing and allocation has gone through a lot of changes?

I did some testing to remove the restriction on x86, for spec2000, I got 1% improvement for three benchmarks: gap, vortex, perlbmk. no regressions seen.
For internal benchmarks, I got 2% improvement for one benchmark and 1% for another. no regressions seen.

[wmi]

Remove the check for all architectures and see if there are performance problems.

[wmi]

Quentin, I will go ahead to checkin the patch and see its peformance impact as you suggested in PR28852 and want to get an explicit approval. Although I only have x86 platforms and only did performance evaluation on them, I remove the check for all the architectures expecting to catch more potential performance problems. I am prepared to analyze those performance problems found on other architectures.

[qcolombet]

Hi Wei,

SGTM.

Cheers,
-Quentin

[qcolombet]

test/CodeGen/X86/MachineSink-SubReg.ll

26 ↗

(On Diff #67264)

use opt -instnamer

llvm/trunk/include/llvm/Target/TargetInstrInfo.h

virtual bool isAsCheapAsAMove(const MachineInstr &MI) const {
     return MI.isAsCheapAsAMove();
   }
 
   /// Return true if the instruction should be sunk by MachineSink.
   ///
   /// MachineSink determines on its own whether the instruction is safe to sink;
   /// this gives the target a hook to override the default behavior with regards
   /// to which instructions should be sunk.
-  /// The default behavior is to not sink insert_subreg, subreg_to_reg, and
-  /// reg_sequence. These are meant to be close to the source to make it easier
-  /// to coalesce.
   virtual bool shouldSink(const MachineInstr &MI) const {
-    return !MI.isInsertSubreg() && !MI.isSubregToReg() && !MI.isRegSequence();
+    return true;
   }
 
   /// Re-issue the specified 'original' instruction at the
   /// specific location targeting a new destination register.
   /// The register in Orig->getOperand(0).getReg() will be substituted by
   /// DestReg:SubIdx. Any existing subreg index is preserved or composed with
   /// SubIdx.
   virtual void reMaterialize(MachineBasicBlock &MBB,

llvm/trunk/test/CodeGen/X86/MachineSink-SubReg.ll

+; PR28852: Check machine code sinking is not stopped by SUBREG_TO_REG.
+; RUN: llc < %s | FileCheck %s
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+; CHECK: foo
+; CHECK-NOT: imull
+; CHECK: retq
+; CHECK: imull
+
+define void @foo(i64 %value, i32 %kLengthBits, i32* nocapture %bits, i64* nocapture %bit_buffer_64, i32 %x) local_unnamed_addr {
+entry:
+  %mul = mul i32 %x, %kLengthBits
+  %add = add i32 %mul, 3
+  %conv = zext i32 %add to i64
+  %mul2 = mul nuw nsw i64 %conv, 5
+  %sub = sub i64 64, %value
+  %conv4 = trunc i64 %sub to i32
+  %tmp0 = load i32, i32* %bits, align 4
+  %cmp = icmp ult i32 %tmp0, %conv4
+  br i1 %cmp, label %if.then, label %if.end, !prof !0
+
+if.then:                                          ; preds = %entry
+  %add7 = add i64 %mul2, %value
+  %tmp1 = load i64, i64* %bit_buffer_64, align 8
+  %add8 = add i64 %add7, %tmp1
+  store i64 %add8, i64* %bit_buffer_64, align 8
+  %conv9 = trunc i64 %mul2 to i32
+  store i32 %conv9, i32* %bits, align 4
+  br label %if.end
+
+if.end:                                           ; preds = %if.then, %entry
+  ret void
+}
+
+!0 = !{!"branch_weights", i32 1, i32 2000}