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

Tom Stellard tom at stellard.net
Thu Feb 14 09:23:31 PST 2013


On Thu, Feb 14, 2013 at 06:07:16PM +0100, Michel Dänzer wrote:
> On Mit, 2013-02-13 at 17:51 +0100, Christian König wrote: 
> > 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.
> 
> [...]
> 
> > > 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.
> 
> I've come up with the patch below, but printInterpSlot doesn't seem to
> end up hooked up anywhere, and indeed P0 is just dumped as '2'. Any idea
> what I'm missing?
> 
> 
> diff --git a/lib/Target/R600/InstPrinter/AMDGPUInstPrinter.cpp b/lib/Target/R600/InstPrinter/AMDGPUInstPrinter.cpp
> index fb17ab7..d6450a0 100644
> --- a/lib/Target/R600/InstPrinter/AMDGPUInstPrinter.cpp
> +++ b/lib/Target/R600/InstPrinter/AMDGPUInstPrinter.cpp
> @@ -40,6 +40,21 @@ void AMDGPUInstPrinter::printOperand(const MCInst *MI, unsigned OpNo,
>    }
>  }
>  
> +void AMDGPUInstPrinter::printInterpSlot(const MCInst *MI, unsigned OpNum,
> +                                        raw_ostream &O) {
> +  unsigned Imm = MI->getOperand(OpNum).getImm();
> +
> +  if (Imm == 2) {
> +    O << "P0";
> +  } else if (Imm == 1) {
> +    O << "P20";
> +  } else if (Imm == 0) {
> +    O << "P10";
> +  } else {
> +    assert(!"Invalid interpolation parameter slot");
> +  }
> +}
> +
>  void AMDGPUInstPrinter::printMemOperand(const MCInst *MI, unsigned OpNo,
>                                          raw_ostream &O) {
>    printOperand(MI, OpNo, O);
> diff --git a/lib/Target/R600/InstPrinter/AMDGPUInstPrinter.h b/lib/Target/R600/InstPrinter/AMDGPUInstPrinter.h
> index e775c4c..767a708 100644
> --- a/lib/Target/R600/InstPrinter/AMDGPUInstPrinter.h
> +++ b/lib/Target/R600/InstPrinter/AMDGPUInstPrinter.h
> @@ -33,6 +33,7 @@ public:
>  
>  private:
>    void printOperand(const MCInst *MI, unsigned OpNo, raw_ostream &O);
> +  void printInterpSlot(const MCInst *MI, unsigned OpNum, raw_ostream &O);
>    void printMemOperand(const MCInst *MI, unsigned OpNo, raw_ostream &O);
>    void printIfSet(const MCInst *MI, unsigned OpNo, raw_ostream &O, StringRef Asm);
>    void printAbs(const MCInst *MI, unsigned OpNo, raw_ostream &O);
> diff --git a/lib/Target/R600/SIISelLowering.cpp b/lib/Target/R600/SIISelLowering.cpp
> index 87cf596..3865d4f 100644
> --- a/lib/Target/R600/SIISelLowering.cpp
> +++ b/lib/Target/R600/SIISelLowering.cpp
> @@ -120,9 +120,6 @@ MachineBasicBlock * SITargetLowering::EmitInstrWithCustomInserter(
>    case AMDGPU::SI_INTERP:
>      LowerSI_INTERP(MI, *BB, I, MRI);
>      break;
> -  case AMDGPU::SI_INTERP_CONST:
> -    LowerSI_INTERP_CONST(MI, *BB, I, MRI);
> -    break;
>    case AMDGPU::SI_WQM:
>      LowerSI_WQM(MI, *BB, I, MRI);
>      break;
> @@ -172,27 +169,6 @@ void SITargetLowering::LowerSI_INTERP(MachineInstr *MI, MachineBasicBlock &BB,
>    MI->eraseFromParent();
>  }
>  
> -void SITargetLowering::LowerSI_INTERP_CONST(MachineInstr *MI,
> -    MachineBasicBlock &BB, MachineBasicBlock::iterator I,
> -    MachineRegisterInfo &MRI) const {
> -  MachineOperand dst = MI->getOperand(0);
> -  MachineOperand attr_chan = MI->getOperand(1);
> -  MachineOperand attr = MI->getOperand(2);
> -  MachineOperand params = MI->getOperand(3);
> -  unsigned M0 = MRI.createVirtualRegister(&AMDGPU::M0RegRegClass);
> -
> -  BuildMI(BB, I, BB.findDebugLoc(I), TII->get(AMDGPU::S_MOV_B32), M0)
> -          .addOperand(params);
> -
> -  BuildMI(BB, I, BB.findDebugLoc(I), TII->get(AMDGPU::V_INTERP_MOV_F32))
> -          .addOperand(dst)
> -          .addOperand(attr_chan)
> -          .addOperand(attr)
> -          .addReg(M0);
> -
> -  MI->eraseFromParent();
> -}
> -
>  void SITargetLowering::LowerSI_V_CNDLT(MachineInstr *MI, MachineBasicBlock &BB,
>      MachineBasicBlock::iterator I, MachineRegisterInfo & MRI) const {
>    unsigned VCC = MRI.createVirtualRegister(&AMDGPU::SReg_64RegClass);
> diff --git a/lib/Target/R600/SIISelLowering.h b/lib/Target/R600/SIISelLowering.h
> index 8528c24..f4bc94d 100644
> --- a/lib/Target/R600/SIISelLowering.h
> +++ b/lib/Target/R600/SIISelLowering.h
> @@ -27,8 +27,6 @@ class SITargetLowering : public AMDGPUTargetLowering {
>                MachineBasicBlock::iterator I, unsigned Opocde) const;
>    void LowerSI_INTERP(MachineInstr *MI, MachineBasicBlock &BB,
>                MachineBasicBlock::iterator I, MachineRegisterInfo & MRI) const;
> -  void LowerSI_INTERP_CONST(MachineInstr *MI, MachineBasicBlock &BB,
> -              MachineBasicBlock::iterator I, MachineRegisterInfo &MRI) const;
>    void LowerSI_WQM(MachineInstr *MI, MachineBasicBlock &BB,
>                MachineBasicBlock::iterator I, MachineRegisterInfo & MRI) const;
>    void LowerSI_V_CNDLT(MachineInstr *MI, MachineBasicBlock &BB,
> diff --git a/lib/Target/R600/SIInstructions.td b/lib/Target/R600/SIInstructions.td
> index 10d5bae..aa67f22 100644
> --- a/lib/Target/R600/SIInstructions.td
> +++ b/lib/Target/R600/SIInstructions.td
> @@ -11,6 +11,20 @@
>  // that are not yet supported remain commented out.
>  //===----------------------------------------------------------------------===//
>  
> +class InterpSlots {
> +int P0 = 2;
> +int P10 = 0;
> +int P20 = 1;
> +}
> +def INTERP : InterpSlots;
> +
> +def InterpSlot : Operand<i32>, PatLeaf<(imm), [{
> +  uint64_t Imm = N->getZExtValue();
> +  return Imm >= 0 && Imm <= 2;
> +}]> {
> +  let PrintMethod = "printInterpSlot";
> +}
> +
>  def isSI : Predicate<"Subtarget.device()"
>                              "->getGeneration() == AMDGPUDeviceInfo::HD7XXX">;
>  
> @@ -683,10 +697,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 InterpSlot:$src0, i32imm:$attr_chan, i32imm:$attr, M0Reg:$m0),

This should be INTERP not InterpSlot, that's probably why the asm
printer hook isn't being called.

-Tom

>    "V_INTERP_MOV_F32",
>    []> {
> -  let VSRC = 0;
>    let DisableEncoding = "$m0";
>  }
>  
> @@ -1081,14 +1094,6 @@ def SI_INTERP : InstSI <
>    []
>  >;
>  
> -def SI_INTERP_CONST : InstSI <
> -  (outs VReg_32:$dst),
> -  (ins i32imm:$attr_chan, i32imm:$attr, SReg_32:$params),
> -  "SI_INTERP_CONST $dst, $attr_chan, $attr, $params",
> -  [(set VReg_32:$dst, (int_SI_fs_interp_constant imm:$attr_chan,
> -                                                 imm:$attr, SReg_32:$params))]
> ->;
> -
>  def SI_WQM : InstSI <
>    (outs),
>    (ins),
> @@ -1324,6 +1329,11 @@ def : Pat <
>  /********** ===================== **********/
>  
>  def : Pat <
> +  (int_SI_fs_interp_constant imm:$attr_chan, imm:$attr, SReg_32:$params),
> +  (V_INTERP_MOV_F32 INTERP.P0, imm:$attr_chan, imm:$attr, SReg_32:$params)
> +>;
> +
> +def : Pat <
>    (int_SI_fs_interp_linear_center imm:$attr_chan, imm:$attr, SReg_32:$params),
>    (SI_INTERP (f32 LINEAR_CENTER_I), (f32 LINEAR_CENTER_J), imm:$attr_chan,
>               imm:$attr, SReg_32:$params)
> 
> 
> -- 
> Earthling Michel Dänzer           |                   http://www.amd.com
> Libre software enthusiast         |          Debian, X and DRI developer
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev


More information about the mesa-dev mailing list