[PATCH][X86] AVX512: Disassembler support for compressed displacement

[Adam Nemet]

Hi Robert,

Thanks very much for the review, I am glad you liked it.  Please see my response inline.

On Jul 16, 2014, at 8:59 AM, Robert Khasanov <rob.khasanov at gmail.com> wrote:

> Hi Adam, 
> 
> I like your idea to replace calculation of CDisp8 scale to TableGen and use it in MC and disassembler. 
> 
> I have notes about your implementation:
> 
> 1. 
> class EVEX_CD8<int esize, CD8VForm form> {
> ...
> +  bits<8> CD8_EltSize = shr<esize, 3>.t;
> ...
>  }
> CD8_EltSize uses only 4 bit (possible values are 1, 2, 4, 8), shrink size of this field.

The main reason to keep all the intermediate values as bits<8> is that I wouldn’t have to cast between them when passing to or from shifts.  I tried using the narrowest type but with all the casting it felt that the expressiveness has really suffered so I went back to bits<8>.  Let me know if you feel strongly about this.

> +  let TSFlags{62-56} = CD8_Scale{6-0};
> Possible values of CD8_Scale are 0, 1, 2, 4, 8, 16, 32 and 64 -  8 different values. I suggest to place this in 3 bits.

I don’t know if you saw it but I had a comment about this:

  // If we run out of TSFlags bits, it's possible to encode this in 3 bits.
  let TSFlags{54-48} = CD8_Scale{6-0};

The main reason I didn’t do this because then I would have had to introduce log2 to tablegen as well.  Again it’s more of a question of taste.  We have plenty of bits and I thought we can introduce the extra complexity only when we really need it.  But just like the previous issue, if you feel strongly about this, I can do it, no problem.

> 2. You added only tests that have compressed displacement. 
> Could you also add tests where displacement is out of range of CDisp8. I didn't find such tests in test/MC/Disassembler/X86/avx-512.txt.

Good point.  I was going to add some but then I forget along the way.  Will do, thanks for noticing.

Adam

> 2014-07-16 4:04 GMT+04:00 Adam Nemet <anemet at apple.com>:
> Hi,
> 
> I am copying a few people directly to make sure this does not get lost among the other AVX512 patches.  There are some design decisions I making here that people may be interested reviewing.
> 
> Currently there is no support for compressed displacement (cdisp8) in the disassembler so we effectively print the wrong (unscaled) displacement value.  The fact that an instruction uses cdisp8 and which form of it is an attribute of the *instruction* rather than the operand.  This fact and the parameters of cdisp8 are specified by deriving an instruction definition from the EVEX_CD8<> class.
> 
> As a consequence when we decode the displacement we decode it according to ENCODE_RM.  This is the encoding type for the operands covered by ModRM.  ENCODING_RM is derived from the type of the operand, e.g. i128mem in i128mem:$src2.  A table is generated by the X86-specific code in tablegen.  It specifies for each operand of each instruction the encoding type and the operand type.
> 
> My approach is to modify the X86 code in tablegen code to consult the instruction attributes *in addition* to the operand type to determine the encoding type.  Thus for example instead of ENCODE_RM, VADDPSZrm will have ENCODE_RM_CD64.  The meaning is that when decoding the displacement, a scale of 64 should be applied to it.
> 
> There is another layering twist to this.  The TD interface to EVEX_CD8 is nice and flexible so there are 32 possible combinations of values for EVEX_CD8<>.  Luckily these all translate into 7 different scaling values.  So clearly to avoid bloating up the encoding type enum we want to include the resulting scaling values in the encoding type rather than the current cdisp8 instruction attributes from TD.
> 
> The assembler (isCDisp8) already has code to compute the cdisp8 scaling value from the attributes.  So we would need to share code between the assembler and tablegen.  I don’t think there is precedence for this but as it turns out it’s not too involved to formulate the logic to compute the scaling factor in TD.  That way both the assembler and the tablegen can use the readily computed value.  Tablegen can use it then to adjust the encoding type from ENCODE_RM to ENCODE_RM_CD<n>.
> 
> TD needed a limited form of the logical left and right shift operators.  I implemented these with the bit-extract/insert operator (i.e. blah{bits}).
> 
> My approach with the patches was to go incremental in order to convince myself that the TD code is in fact equivalent to the original C++ code.
> 
> Please let me know if this looks OK.
> 
> Adam
> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> 
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140716/5f84f604/attachment.html>