SIScheduler should track lane masks

[alex-t]

Summary

On subreg COPY operation the not-clobbered subreg of the destination register is considered to be implicitly used.
For example: on "%vreg1.sub0 = COPY %vreg2" >> %vreg1.sub1 is implicit Use.
W/o lane lane masks tracking, destination register of the COPY is considered live in in current scheduling block. Here scheduling block has meaning as described in SIScheduler design.
Since it is not live out in any of the predecessors we end up in assert.

[axeldavy]

If the patch fixes a crash, why is it abandonned ?

[alex-t]

There is a better way to enforce correct sub-register liveness tracking.
I should better switch EnableSIScheduler subtarget feature ON so that the liveness information is already correct at the point where RPTracker uses it.

[alex-t]

SIMachineScheduler really treat not-clobbered sub-registers as an implicit uses and produce incorrect liv in set for the scheduling block.
Some time ago someone added a condition to the SISubtarget::overrideSchedPolicy function:
if (!enableSIScheduler())

Policy.ShouldTrackLaneMasks = true;
So, the correct answer is:
in case sub-registers liveness a tracked SIScheduleDAGMI::initRPTracker should call init with TrackLaneMasks = true
Now we have a bug in SIScheduleDAGMI that was covered with a lame fix mentioned above.

[rampitec]

Is there any test which crashes without it? Can you add one?

lib/Target/AMDGPU/AMDGPUSubtarget.cpp

263

Should be better to use enableSubRegLiveness() query.

lib/Target/AMDGPU/SIMachineScheduler.h

443–444

Same here.

[arsenm]

More tests are definitely needed. SIScheduler currently is the worst tested file in the backend: http://llvm.org/reports/coverage/lib/Target/AMDGPU/index-sort-l.html

[alex-t]

In D29442#664926, @arsenm wrote:

More tests are definitely needed. SIScheduler currently is the worst tested file in the backend: http://llvm.org/reports/coverage/lib/Target/AMDGPU/index-sort-l.html

I'm not going to switch it ON by default. BTW we have no command line option for that - just Subtarget Ctor initialized with 'false' for this feature.
I ran it on SGEMM and experienced no crashes. That is a kind of good testing itself.

[rampitec]

In D29442#664929, @alex-t wrote:

In D29442#664926, @arsenm wrote:

More tests are definitely needed. SIScheduler currently is the worst tested file in the backend: http://llvm.org/reports/coverage/lib/Target/AMDGPU/index-sort-l.html

I'm not going to switch it ON by default. BTW we have no command line option for that - just Subtarget Ctor initialized with 'false' for this feature.
I ran it on SGEMM and experienced no crashes. That is a kind of good testing itself.

As far as I can see si-scheduler.ll enables it with --misched=si -mattr=si-scheduler. A test with si scheduler enabled to check it does not fail would be desirable.

[alex-t]

Updated according to reviewers comments

[rampitec]

LGTM.

[axeldavy]

I have hard time thinking what differences the subreg tracking causes.

The reason I had advised to disable it with this scheduler when subreg tracking was introduced is I couldn't exactly figure out what are the implications of tracking subregs.

Possibly tracking subregs require as well changes to initRegPressure (Are the assumptions at the end still valid ?), or introduce changes to the way the scheduler works to track subregs as well instead of regs.

Could you explain why your patch is sufficient ?

[alex-t]

This is a temporary solution. Given that nobody uses SI misched except AMD I had a hope that this change does not break anything.
The aim of this patch was to fix concrete crash to make possible investigate the SI misched behavior on concrete cases.
We need this to find out is this scheduler can solve specific problems.

Register tracking algorithm need to be revised if we want count lane masks. This is obvious.
Now I'm looking on the another crash due to the similar reason. I'm going to come up with the real patch soon.
As for this one I'd prefer to keep it in case it doesn't break things for you.

[axeldavy]

I suspect it causes a regression on several games.

https://bugs.freedesktop.org/show_bug.cgi?id=99748

The user is currently compiling with this patch reverted, and we'll have confirmation it causes the regression.

[axeldavy]

The user says with the patch reverted, things work again.

[axeldavy]

I believe adding lanemask tracking to sisched will be non trivial work (and I think it would complexify the code, which is already complex).

Basically the scheduler does its own tracking (since it regroups instructions into blocks, and you want to track how scheduling a given block would affect register usage).
Thus its own tracking should be adapted to take lanemask into account.

I'm afraid turning RPTracker's lanemask tracking on breaks sisched assumptions when it builds its blocks.

With lanemask tracking disabled, it's not supposed to crash. Perhaps we could we look at fixing the example you reported ?
Which assert is raised ? The one in decreaseLiveRegs ?
The case you describe is weird, because the LiveIn registers are initialised with the registers that the RP tracker finds are used without being defined.
Thus in your example, it shouldn't crash.

[alex-t]

In D29442#674625, @axeldavy wrote:

With lanemask tracking disabled, it's not supposed to crash. Perhaps we could we look at fixing the example you reported ?
Which assert is raised ? The one in decreaseLiveRegs ?
The case you describe is weird, because the LiveIn registers are initialised with the registers that the RP tracker finds are used without being defined.
Thus in your example, it shouldn't crash.

"%vreg1.sub0 = COPY %vreg2"

%vreg1.sub0 is defined
%vreg1.sub1 is implicitly used
w/o sub-registers tracking %vreg1 is cinsidered as used but not defined in the current block i.e. live in. Same time it is not live out in any of predecessors so we've got an assert in decreaseLiveRegs.

My question is: why do we count live ins in such manner? Why we look at the "used but not defined"? In my understanding, Live in is a register that is live out in any of the predecessors and that's it. Any other ways to count live ins look odd to me.

[alex-t]

patch reverted
r295054 git svn

[axeldavy]

It looks like with --misched=si, the lane mask tracking is enabled, while with -mattr=si-scheduler it isn't.
That may explain why you were seing the crash in the first place, perhaps you were using --misched=si.

lib/Target/AMDGPU/AMDGPUSubtarget.cpp

void SISubtarget::overrideSchedPolicy(MachineSchedPolicy &Policy,
   // SIRegisterInfo::getRegPressureSetLimit()
   Policy.ShouldTrackPressure = true;
 
   // Enabling both top down and bottom up scheduling seems to give us less
   // register spills than just using one of these approaches on its own.
   Policy.OnlyTopDown = false;
   Policy.OnlyBottomUp = false;
 
-  // Enabling ShouldTrackLaneMasks crashes the SI Machine Scheduler.
-  if (!enableSIScheduler())
-    Policy.ShouldTrackLaneMasks = true;
+  Policy.ShouldTrackLaneMasks = enableSubRegLiveness();
 }
 
 bool SISubtarget::isVGPRSpillingEnabled(const Function& F) const {
   return EnableVGPRSpilling || !AMDGPU::isShader(F.getCallingConv());
 }
 
 unsigned SISubtarget::getKernArgSegmentSize(const MachineFunction &MF,
               unsigned ExplicitArgBytes) const {

lib/Target/AMDGPU/SIMachineScheduler.h

public:
 
   ~SIScheduleDAGMI() override;
 
   // Entry point for the schedule.
   void schedule() override;
 
   // To init Block's RPTracker.
   void initRPTracker(RegPressureTracker &RPTracker) {
-    RPTracker.init(&MF, RegClassInfo, LIS, BB, RegionBegin, false, false);
+    RPTracker.init(&MF, RegClassInfo, LIS, BB, RegionBegin,
+     MF.getSubtarget().enableSubRegLiveness(), false);
   }
 
   MachineBasicBlock *getBB() { return BB; }
   MachineBasicBlock::iterator getCurrentTop() { return CurrentTop; }
   MachineBasicBlock::iterator getCurrentBottom() { return CurrentBottom; }
   LiveIntervals *getLIS() { return LIS; }
   MachineRegisterInfo *getMRI() { return &MRI; }
   const TargetRegisterInfo *getTRI() { return TRI; }

test/CodeGen/AMDGPU/si-shed-track-subregs.ll

+; This test does not check anything. Just ensure no crash.
+; RUN: llc -O2 -mtriple amdgcn--amdhsa --misched=si -mattr=si-scheduler -mcpu=fiji -filetype=asm < %s
+
+declare i32 @llvm.amdgcn.workitem.id.x() #4
+
+declare i32 @llvm.amdgcn.workitem.id.y() #4
+
+define amdgpu_kernel void @"test"(float addrspace(1)* nocapture,
+ [4 x [4 x float]] addrspace(3) *, 
+ [4 x [4 x float]] addrspace(3) *,
+ [4 x [4 x float]] addrspace(3) *,
+ [4 x [4 x float]] addrspace(3) *
+) {
+
+  %st_addr = getelementptr float, float addrspace(1)* %0, i64 10
+  %id_x = tail call i32 @llvm.amdgcn.workitem.id.x() #4
+  %id_y = tail call i32 @llvm.amdgcn.workitem.id.y() #4
+
+  %6  = getelementptr [4 x [4 x float]], [4 x [4 x float]] addrspace(3)* %1, i32 0, i32 %id_y, i32 1234
+  %7  = getelementptr [4 x [4 x float]], [4 x [4 x float]] addrspace(3)* %2, i32 0, i32 0, i32 %id_x
+  %8  = getelementptr [4 x [4 x float]], [4 x [4 x float]] addrspace(3)* %3, i32 0, i32 %id_y, i32 0
+  %9  = getelementptr [4 x [4 x float]], [4 x [4 x float]] addrspace(3)* %4, i32 0, i32 0, i32 %id_x
+  %10 = getelementptr [4 x [4 x float]], [4 x [4 x float]] addrspace(3)* %1, i32 0, i32 %id_y, i32 1294
+  %11 = getelementptr [4 x [4 x float]], [4 x [4 x float]] addrspace(3)* %2, i32 0, i32 1, i32 %id_x
+  %12 = getelementptr [4 x [4 x float]], [4 x [4 x float]] addrspace(3)* %3, i32 0, i32 %id_y, i32 1
+  %13 = getelementptr [4 x [4 x float]], [4 x [4 x float]] addrspace(3)* %4, i32 0, i32 1, i32 %id_x
+
+  
+  %14 = load float, float addrspace(3)* %6
+  %15 = load float, float addrspace(3)* %7
+  %mul3 = fmul float %14, %15
+  %add1 = fadd float 2.0, %mul3
+  %16 = load float, float addrspace(3)* %8
+  %17 = load float, float addrspace(3)* %9
+  %mul4 = fmul float %16, %17
+  %sub2 = fsub float %add1, %mul4
+  %18 = load float, float addrspace(3)* %10
+  %19 = load float, float addrspace(3)* %11
+  %mul5 = fmul float %18, %19
+  %sub3 = fsub float %sub2, %mul5
+  %20 = load float, float addrspace(3)* %12
+  %21 = load float, float addrspace(3)* %13
+  %mul6 = fmul float %20, %21
+  %sub4 = fsub float %sub3, %mul6
+  store float %sub4, float addrspace(1)* %st_addr
+  ret void
+}
+
+attributes #4 = { nounwind readnone }