[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