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

[Robert Khasanov]

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.

+  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.

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.



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/f6ca8cfa/attachment.html>