[PATCH][X86] AVX512: Disassembler support for compressed displacement
Robert Khasanov
rob.khasanov at gmail.com
Thu Jul 17 03:17:52 PDT 2014
2014-07-17 2:16 GMT+04:00 Adam Nemet <anemet at apple.com>:
>
> On Jul 16, 2014, at 2:43 PM, Adam Nemet <anemet at apple.com> wrote:
>
>
> On Jul 16, 2014, at 2:25 PM, Robert Khasanov <rob.khasanov at gmail.com>
> wrote:
>
> 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.
>
>
> Ah, nicely hidden :(.
> http://llvm.org/docs/TableGen/LangIntro.html#tablegen-values-and-expressions
> says nothing about these. Anyhow, good catch. I’ll go an update the
> patch to use these.
>
>
> Actually, looks like these don’t work for bits<n>:
>
> /org/llvm/build$ cat /tmp/s.td
> def s {
> bits<3> b = { 0, 1, 0};
> int i = 2;
> int shifted_b = !shl(b, 2);
> int shifted_i = !shl(i, 2);
> }
> /org/llvm/build$ ./Debug+Asserts/bin/llvm-tblgen /tmp/s.td
> ------------- Classes -----------------
> ------------- Defs -----------------
> def s {
> bits<3> b = { 0, 1, 0 };
> int i = 2;
> int shifted_b = !shl({ 0, 1, 0 }, 2);
> int shifted_i = 8;
> string NAME = ?;
> }
>
>
> The fact that they are not evaluated causes problems when the bits are
> stuffed into TSFlags:
>
> llvm[3]: Building X86.td disassembly tables with tblgen
> error:Invalid TSFlags bit in Int_VCOMISDZrm
>
> Anyhow, I think I have a fix to tablegen to get bits<n> supported but
> these patches would have to wait until then.
>
> Adam
>
>
Oh, I see. While it is not fixed in TableGen, you can use temporal
solution: convert bits<n> to int through new variable and use it in shift
operators. See example below.
llvm> cat /tmp/s.td
def s {
bits<3> b = { 0, 1, 0};
int b_cast = b;
int i = 5;
int shifted_b = !shl(b_cast, 2);
int shifted_i = !shl(i, 2);
}
llvm> ../build/Debug+Asserts/bin/llvm-tblgen /tmp/s.td
------------- Classes -----------------
------------- Defs -----------------
def s {
bits<3> b = { 0, 1, 0 };
int b_cast = 2;
int i = 5;
int shifted_b = 8;
int shifted_i = 20;
string NAME = ?;
}
>> + 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.
>
>
> Now that we have evidence that tablegen is not against introducing integer
> arithmetic, I may come back to this an impelement !log2 and then we can
> revisit this.
>
> I am planning to commit with these after updating the shifts unless there
> are further comments. Thanks again.
>
> Adam
>
>
> 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/a34c9a00/attachment.html>
More information about the llvm-commits
mailing list