[Mesa-dev] [PATCH v2] R600/SI: Fix INTERP_CONST.

[Christian König]

Am 13.02.2013 17:07, schrieb Michel Dänzer:
> From: Michel Dänzer <michel.daenzer at amd.com>
>
> The important fix is that the constant interpolation value is stored in the
> parameter slot P0, which is encoded as 2.
>
> In addition, pass the parameter slot as an operand to V_INTERP_MOV_F32
> instead of hardcoding it there, and add a special register class for the
> parameter slots for type checking and pretty dumping.
>
> NOTE: This is a candidate for the Mesa stable branch.
>
> Signed-off-by: Michel Dänzer <michel.daenzer at amd.com>
> ---
>
> v2: Mark parameter slot registers as reserved.
>
>   lib/Target/R600/AMDGPUAsmPrinter.cpp | 3 +++
>   lib/Target/R600/SIISelLowering.cpp   | 1 +
>   lib/Target/R600/SIInstructions.td    | 3 +--
>   lib/Target/R600/SIRegisterInfo.cpp   | 5 +++++
>   lib/Target/R600/SIRegisterInfo.td    | 4 ++++
>   5 files changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/lib/Target/R600/AMDGPUAsmPrinter.cpp b/lib/Target/R600/AMDGPUAsmPrinter.cpp
> index 254e62e..544aed2 100644
> --- a/lib/Target/R600/AMDGPUAsmPrinter.cpp
> +++ b/lib/Target/R600/AMDGPUAsmPrinter.cpp
> @@ -92,6 +92,9 @@ void AMDGPUAsmPrinter::EmitProgramInfo(MachineFunction &MF) {
>           default: break;
>           case AMDGPU::EXEC:
>           case AMDGPU::M0:
> +        case AMDGPU::P0:
> +        case AMDGPU::P10:
> +        case AMDGPU::P20:
>             continue;
>           }
>   
> diff --git a/lib/Target/R600/SIISelLowering.cpp b/lib/Target/R600/SIISelLowering.cpp
> index 92de379..c0b04e5 100644
> --- a/lib/Target/R600/SIISelLowering.cpp
> +++ b/lib/Target/R600/SIISelLowering.cpp
> @@ -186,6 +186,7 @@ void SITargetLowering::LowerSI_INTERP_CONST(MachineInstr *MI,
>   
>     BuildMI(BB, I, BB.findDebugLoc(I), TII->get(AMDGPU::V_INTERP_MOV_F32))
>             .addOperand(dst)
> +          .addReg(AMDGPU::P0)
>             .addOperand(attr_chan)
>             .addOperand(attr)
>             .addReg(M0);
> diff --git a/lib/Target/R600/SIInstructions.td b/lib/Target/R600/SIInstructions.td
> index feb0188..d4f7b37 100644
> --- a/lib/Target/R600/SIInstructions.td
> +++ b/lib/Target/R600/SIInstructions.td
> @@ -681,10 +681,9 @@ def V_INTERP_P2_F32 : VINTRP <
>   def V_INTERP_MOV_F32 : VINTRP <
>     0x00000002,
>     (outs VReg_32:$dst),
> -  (ins i32imm:$attr_chan, i32imm:$attr, M0Reg:$m0),
> +  (ins ParamReg:$src0, i32imm:$attr_chan, i32imm:$attr, M0Reg:$m0),
>     "V_INTERP_MOV_F32",
>     []> {
> -  let VSRC = 0;
>     let DisableEncoding = "$m0";
>   }
>   
> diff --git a/lib/Target/R600/SIRegisterInfo.cpp b/lib/Target/R600/SIRegisterInfo.cpp
> index 88275c5..4e7cbf9 100644
> --- a/lib/Target/R600/SIRegisterInfo.cpp
> +++ b/lib/Target/R600/SIRegisterInfo.cpp
> @@ -27,6 +27,11 @@ SIRegisterInfo::SIRegisterInfo(AMDGPUTargetMachine &tm,
>   
>   BitVector SIRegisterInfo::getReservedRegs(const MachineFunction &MF) const {
>     BitVector Reserved(getNumRegs());
> +
> +  Reserved.set(AMDGPU::P0);
> +  Reserved.set(AMDGPU::P10);
> +  Reserved.set(AMDGPU::P20);
> +
>     return Reserved;
>   }
>   
> diff --git a/lib/Target/R600/SIRegisterInfo.td b/lib/Target/R600/SIRegisterInfo.td
> index ab36b87..46c8f91 100644
> --- a/lib/Target/R600/SIRegisterInfo.td
> +++ b/lib/Target/R600/SIRegisterInfo.td
> @@ -23,6 +23,9 @@ def EXEC_HI : SIReg <"EXEC HI", 127>;
>   def EXEC : SI_64<"EXEC", [EXEC_LO, EXEC_HI], 126>;
>   def SCC : SIReg<"SCC", 253>;
>   def M0 : SIReg <"M0", 124>;
> +def P10 : SIReg <"P10", 0>;
> +def P20 : SIReg <"P20", 1>;
> +def P0 : SIReg <"P0", 2>;

I'm not sure if representing constants as registers is such a good idea. 
>From the POV of the selection DAG a register is primary the destination 
of an operation.

Maybe using an Operand<i32> with a proper "PrintMethod" set might be 
better, take a look at ARMInstPrinter::printThumbSRImm on how that's done.

Christian.

>   
>   //Interpolation registers
>   def PERSP_SAMPLE_I : SIReg <"PERSP_SAMPLE_I">;
> @@ -187,4 +190,5 @@ def SCCReg : RegisterClass<"AMDGPU", [i1], 1, (add SCC)>;
>   def VCCReg : RegisterClass<"AMDGPU", [i1], 1, (add VCC)>;
>   def EXECReg : RegisterClass<"AMDGPU", [i1], 1, (add EXEC)>;
>   def M0Reg : RegisterClass<"AMDGPU", [i32], 32, (add M0)>;
> +def ParamReg : RegisterClass<"AMDGPU", [i32], 32, (add P0, P10, P20)>;
>