[PATCH][X86] AVX512: Disassembler support for compressed displacement
Adam Nemet
anemet at apple.com
Thu Jul 17 10:36:30 PDT 2014
On Jul 17, 2014, at 3:17 AM, Robert Khasanov <rob.khasanov at gmail.com> wrote:
>
>
>
> 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 = ?;
> }
We’re good. Hal gave his OK to the tablegen fix. Committed as r213278, r213279, r213280 and r213281.
Thanks for looking at this and for the !shl catch!
Adam
>>>
>>>> + 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/782469b9/attachment.html>
More information about the llvm-commits
mailing list