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

[Robert Khasanov]

Adam,

Please see my response inline.

2014-07-16 21:08 GMT+04:00 Adam Nemet <anemet at apple.com>:

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

I found that TableGen already has shift operators !shl, !sra and !srl. They
are listed in http://llvm.org/docs/TableGen/LangRef.html#lexical-analysis.
I think it would be better to use this operators.


>
> +  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.
>
>
Sorry, I didn't see your comment.
I don't know exactly how much it is critical. I just see that other TSFlags
are used compactly.

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/20140717/53564eeb/attachment.html>