[Mesa-dev] [PATCH 09/14] R600/SI: add all the other missing asm operands

[Michel Dänzer]

On Don, 2013-02-21 at 10:38 +0100, Christian König wrote: 
> Am 21.02.2013 09:39, schrieb Michel Dänzer:
> > On Mit, 2013-02-20 at 18:46 +0100, Christian König wrote:
> >> diff --git a/lib/Target/R600/SIInstructions.td b/lib/Target/R600/SIInstructions.td
> >> index 700b8f8..866c7cb 100644
> >> --- a/lib/Target/R600/SIInstructions.td
> >> +++ b/lib/Target/R600/SIInstructions.td
> >> @@ -620,7 +620,7 @@ def V_INTERP_P1_F32 : VINTRP <
> >>     0x00000000,
> >>     (outs VReg_32:$dst),
> >>     (ins VReg_32:$i, i32imm:$attr_chan, i32imm:$attr, M0Reg:$m0),
> >> -  "V_INTERP_P1_F32",
> >> +  "V_INTERP_P1_F32 $dst, $i, $attr_chan, $attr, $m0",
> > I didn't put $m0 in the V_INTERP_MOV_F32 asm string because it's not an
> > explicit operand but used implicitly. I can see the value in printing it
> > anyway, but it should probably be distinguished from the explicit
> > operands somehow, e.g. "[$m0]" or something like that.
> 
> Any suggestion for the full asm string?

E.g.

  "V_INTERP_P1_F32 $dst, $i, $attr_chan, $attr, [$m0]"


> >> @@ -722,16 +722,19 @@ def S_WAITCNT : SOPP <0x0000000c, (ins i32imm:$simm16), "S_WAITCNT $simm16",
> >>   //def S_TTRACEDATA : SOPP_ <0x00000016, "S_TTRACEDATA", []>;
> >>   
> >>   def V_CNDMASK_B32_e32 : VOP2 <0x00000000, (outs VReg_32:$dst),
> >> -  (ins VSrc_32:$src0, VReg_32:$src1, VCCReg:$vcc), "V_CNDMASK_B32_e32",
> >> +  (ins VSrc_32:$src0, VReg_32:$src1, VCCReg:$vcc),
> >> +  "V_CNDMASK_B32_e32 $dst, $src0, $src1, $vcc",
> >>     []
> >>   >{
> >>     let DisableEncoding = "$vcc";
> > These asm strings should be made consistent with whichever way we decide
> > to handle implicit operands as well.
> 
> I left out the implicit regs for the S_CBRANCH_* instruction because 
> they are part of the instruction name anyway, but for everything else I 
> don't see any reason why we shouldn't print them.
> 
> For the example of V_CNDMASK_B32_e32  I would say that we should 
> definitely print it, cause that bring us in line with the _e64 encoding 
> which can have the condition in any SReg_64.

Right. Along similar lines, printing them for S_CBRANCH_* might help
catch bugs which somehow cause the register allocator to try and use a
different register.


> > Other than these nits, the series looks good to me. Be sure to run make
> > check, in case the lit tests need to be updated for the asm string
> > changes.
> 
> I'm usually only running "llvm-lit test/CodeGen/R600/", cause I don't 
> build anything else than the R600 target and so running all the other 
> tests shouldn't make much sense.

That's probably fine, though make check takes less than a minute on my
quad core Llano.


-- 
Earthling Michel Dänzer           |                   http://www.amd.com
Libre software enthusiast         |          Debian, X and DRI developer