[llvm-commits] XOP encoding patch

[Bruno Cardoso Lopes]

Hi,

On Thu, Dec 8, 2011 at 7:06 PM, Eli Friedman <eli.friedman at gmail.com> wrote:
> On Thu, Dec 8, 2011 at 12:49 PM, Jan Sjodin <jan_sjodin at yahoo.com> wrote:
>> This patch handles the encoding of XOP instructions. I included one
>> instruction for each kind including a test.
>
> +multiclass xop4opimm<bits<8> opc, string OpcodeStr> {
> +  def ri : IXOPi8<opc, MRMSrcReg, (outs VR128:$dst),
> +           (ins VR128:$src1, VR128:$src2, i32i8imm:$src3),
>
> As discussed on IRC, you want i8imm here, not i32i8imm.
>
> +      // If there is an additional 5th operand it must be an immediate, which
> +      // is encoded in bits[3:0]
> +      if(CurOp != NumOps) {
> +       const MCOperand &MIMM = MI.getOperand(CurOp++);
> +       if(MIMM.isImm()) {
> +         unsigned Val = MIMM.getImm();
> +         assert(Val < 16 && "Immediate operand value out of range");
> +         RegNum |= Val;
> +       }
> +      }
>
> No tabs, please.
>
> This patch doesn't include disassembler support; are you planning to add that?
>
> Someone more familiar with the x86 instruction patterns should
> probably review this in addition.

Regarding the MC code emitter, my suggestion here is to factor out
common code from EmitVEXOpcodePrefix, and have a new
EmitXOPOpcodePrefix method which reuses the common part. IMO adding
several tricky conditions to get around the differences between them
isn't the way to go (and compromisses the code readability). That
means whenever one of the VEX prefixes has a slightly different
meaning in XOP, there should be a new TSFlags for it.

-- 
Bruno Cardoso Lopes
http://www.brunocardoso.cc