[AMDGPU] For amdpal, widen interpolation mode workaround

[tpr]

Summary

The interpolation mode workaround ensures that at least one
interpolation mode is enabled in PSInputAddr. It does not also check
PSInputEna on the basis that the user might enable bits in that
depending on run-time state.

However, for amdpal os type, the user does not enable some bits after
compilation based on run-time states; the register values being
generated here are the final ones set in the hardware. Therefore, apply
the workaround to PSInputAddr and PSInputEnable together. (The case
where a bit is set in PSInputAddr but not in PSInputEnable is where the
frontend set up an input arg for a particular interpolation mode, but
nothing uses that input arg. Really we should have an earlier pass that
removes such an arg.)

[arsenm]

test/CodeGen/AMDGPU/amdpal-psenable.ll

21 ↗

(On Diff #114885)

Can we rename these attributes to match the naming convention and start with an amdgpu- prefix

[tpr]

test/CodeGen/AMDGPU/amdpal-psenable.ll

21 ↗

(On Diff #114885)

Maybe, but that is unrelated to what this change is about. I suggest doing it in a different change, Also it would need coordination between the backend, mesa, and now the amdpal frontend.

[tpr]

Ping.

(We don't want to rename those attributes in this change. This change did not add the attributes, and they are already in use by Mesa.)

[nhaehnle]

lib/Target/AMDGPU/SIISelLowering.cpp

1511–1517 ↗

(On Diff #114885)

Could you make this guarded by isAmdPalOS? For Mesa, it's possible that another shader part will enable some of those input bits, so we want to set those enables only after the combined enable mask is known.

[tpr]

lib/Target/AMDGPU/SIISelLowering.cpp

1511–1517 ↗

(On Diff #114885)

Oops, it was guarded by isAmdPalOS in my first local version of this change. But I seem to have lost it.

I will re-add the guard.

[tpr]

Addressed review comment.

[tpr]

Oops, I have still failed to protect it in isAmdPalOS. Back in a minute with a real fix.

[tpr]

Really address review comment.

[nhaehnle]

LGTM

llvm/trunk/lib/Target/AMDGPU/SIISelLowering.cpp

if (IsShader) {
     // based on run-time states. Since we can't know what the final PSInputEna
     // will look like, so we shouldn't do anything here and the user should take
     // responsibility for the correct programming.
     //
     // Otherwise, the following restrictions apply:
     // - At least one of PERSP_* (0xF) or LINEAR_* (0x70) must be enabled.
     // - If POS_W_FLOAT (11) is enabled, at least one of PERSP_* must be
     //   enabled too.
-    if (CallConv == CallingConv::AMDGPU_PS &&
-        ((Info->getPSInputAddr() & 0x7F) == 0 ||
+    if (CallConv == CallingConv::AMDGPU_PS) {
+      if ((Info->getPSInputAddr() & 0x7F) == 0 ||
            ((Info->getPSInputAddr() & 0xF) == 0 &&
-          Info->isPSInputAllocated(11)))) {
+            Info->isPSInputAllocated(11))) {
         CCInfo.AllocateReg(AMDGPU::VGPR0);
         CCInfo.AllocateReg(AMDGPU::VGPR1);
         Info->markPSInputAllocated(0);
         Info->markPSInputEnabled(0);
       }
+      if (Subtarget->isAmdPalOS()) {
+        // For isAmdPalOS, the user does not enable some bits after compilation
+        // based on run-time states; the register values being generated here are
+        // the final ones set in hardware. Therefore we need to apply the
+        // workaround to PSInputAddr and PSInputEnable together.  (The case where
+        // a bit is set in PSInputAddr but not PSInputEnable is where the
+        // frontend set up an input arg for a particular interpolation mode, but
+        // nothing uses that input arg. Really we should have an earlier pass
+        // that removes such an arg.)
+        unsigned PsInputBits = Info->getPSInputAddr() & Info->getPSInputEnable();
+        if ((PsInputBits & 0x7F) == 0 ||
+            ((PsInputBits & 0xF) == 0 &&
+             (PsInputBits >> 11 & 1)))
+          Info->markPSInputEnabled(
+              countTrailingZeros(Info->getPSInputAddr(), ZB_Undefined));
+      }
+    }
 
     assert(!Info->hasDispatchPtr() &&
            !Info->hasKernargSegmentPtr() && !Info->hasFlatScratchInit() &&
            !Info->hasWorkGroupIDX() && !Info->hasWorkGroupIDY() &&
            !Info->hasWorkGroupIDZ() && !Info->hasWorkGroupInfo() &&
            !Info->hasWorkItemIDX() && !Info->hasWorkItemIDY() &&
            !Info->hasWorkItemIDZ());
   } else if (IsKernel) {

llvm/trunk/test/CodeGen/AMDGPU/amdpal-psenable.ll

+; RUN: llc -mtriple=amdgcn--amdpal -verify-machineinstrs < %s | FileCheck -check-prefix=GCN -check-prefix=SI %s
+; RUN: llc -mtriple=amdgcn--amdpal -mcpu=tonga -verify-machineinstrs < %s | FileCheck -check-prefix=GCN -check-prefix=VI %s
+; RUN: llc -mtriple=amdgcn--amdpal -mcpu=gfx900 -verify-machineinstrs < %s | FileCheck -check-prefix=GCN -check-prefix=GFX9 -enable-var-scope %s
+
+; This pixel shader does not use the result of its interpolation, so it would
+; end up with an interpolation mode set in PSAddr but not PSEnable. This test tests
+; the workaround that ensures that an interpolation mode is also set in PSEnable.
+; GCN-LABEL: {{^}}amdpal_psenable:
+; GCN: .amd_amdgpu_pal_metadata{{.*}}0xa1b3,0x2,0xa1b4,0x2,
+define amdgpu_ps void @amdpal_psenable(i32 inreg, i32 inreg, i32 inreg, i32 inreg %m0, <2 x float> %pos) #6 {
+  %inst23 = extractelement <2 x float> %pos, i32 0
+  %inst24 = extractelement <2 x float> %pos, i32 1
+  %inst25 = tail call float @llvm.amdgcn.interp.p1(float %inst23, i32 0, i32 0, i32 %m0)
+  %inst26 = tail call float @llvm.amdgcn.interp.p2(float %inst25, float %inst24, i32 0, i32 0, i32 %m0)
+  ret void
+}
+
+declare float @llvm.amdgcn.interp.p1(float, i32, i32, i32) #2
+declare float @llvm.amdgcn.interp.p2(float, float, i32, i32, i32) #2
+
+attributes #6 = { nounwind "InitialPSInputAddr"="2" }