<div dir="ltr">All 4 look fine to me as well.<br><br>And yes, update llvm-mode.el and/or llvm.vim ;)<div><br></div><div>Thanks!</div><div><br></div><div>-eric</div></div><br><div class="gmail_quote">On Thu Feb 12 2015 at 4:18:17 PM Adrian Prantl <<a href="mailto:aprantl@apple.com">aprantl@apple.com</a>> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
> On Feb 12, 2015, at 4:07 PM, Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>> wrote:<br>
><br>
><br>
>> On 2015-Feb-11, at 08:25, Adrian Prantl <<a href="mailto:aprantl@apple.com" target="_blank">aprantl@apple.com</a>> wrote:<br>
>><br>
>>><br>
>>> On Feb 10, 2015, at 6:29 PM, Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>> wrote:<br>
>>><br>
>>>><br>
>>>> On 2015-Feb-10, at 16:59, Adrian Prantl <<a href="mailto:aprantl@apple.com" target="_blank">aprantl@apple.com</a>> wrote:<br>
>>>><br>
>>>><br>
>>>>> On Feb 10, 2015, at 4:48 PM, Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>> wrote:<br>
>>>>><br>
>>>>><br>
>>>>>> On 2015-Feb-10, at 16:05, Adrian Prantl <<a href="mailto:aprantl@apple.com" target="_blank">aprantl@apple.com</a>> wrote:<br>
>>>>>><br>
>>>>>> A few nitpicky suggestions and questions:<br>
>>>>>><br>
>>>>>> - !MDSubrange(count: 30, lo: 2)<br>
>>>>>> what about spelling out “low” and “high”?<br>
>>>>><br>
>>>>> It doesn't mean "low" and "high", though, right? It means<br>
>>>>> "count"/"size" and "index of first element”.<br>
>>>><br>
>>>> You are probably right. I was thinking of DWARF where a subrange may either have a<br>
>>>> DW_AT_lower_bound and/or a DW_AT_upper_bound<br>
>>>> or a<br>
>>>> DW_AT_lower_bound and/or a DW_AT_count<br>
>>>><br>
>>>> We only support the second form so count and lower_bound would probably be most accurate.<br>
>>><br>
>>> Sounds good. In keeping with the field naming convention I've adopted,<br>
>>> I'll use 'count:' and 'lowerBound:’.<br>
>><br>
>> yes, sure!<br>
>>><br>
>>>>><br>
>>>>> Or have I misinterpreted it?<br>
>>>>><br>
>>>>> How about "count" and "low"?<br>
>>>>><br>
>>>>>><br>
>>>>>> - do we still need the uniqueID on MDLexicalBlock? Could it be a distinct node instead?<br>
>>>>><br>
>>>>> I think this should be distinct instead. I was planning to do this<br>
>>>>> as part of the upgrade (moving them into place).<br>
>>>>><br>
>>>>>><br>
>>>>>> - is it possible to have it print DW_OP-* constants such as in<br>
>>>>>> !MDExpression(DW_OP_deref, DW_OP_bit_piece, 0, 8)<br>
>>>>>> ?<br>
>>>>><br>
>>>>> I think this is pretty difficult the way that expressions are stored<br>
>>>>> right now -- it requires knowing how many arguments each thing takes,<br>
>>>>> etc. In particular, how would the `AsmWriter` know whether something<br>
>>>>> should have a symbolic constant (`DW_OP_deref`) vs. being the number 6?<br>
>>>><br>
>>>> It would have to reuse the code from DIExpression::printInternal().<br>
>>><br>
>>> I think this is doable.<br>
>>><br>
>>> On the LLParser side, I've generally allowed raw ints (for `DW_TAG*`,<br>
>>> `DW_LANG*`, etc.), but for `DW_OP`s we could (at least for now) require<br>
>>> symbolic constants.<br>
>>><br>
>>>>><br>
>>>>> I was thinking of trying to clean this up later, but since we're<br>
>>>>> talking about it:<br>
>>>>><br>
>>>>> I think instead of `std::vector<uint64_t>` it should be<br>
>>>>> `std::vector<ExprOperand>`, where:<br>
>>>>><br>
>>>>> struct ExprOperand {<br>
>>>>> LocationAtom Kind;<br>
>>>>> unsigned NumArgs;<br>
>>>>> uint64_t Args[2];<br>
>>>>> ExprOperand(LocationAtom Kind);<br>
>>>>> ExprOperand(LocationAtom Kind, uint64_t Arg);<br>
>>>>> ExprOperand(LocationAtom Kind, uint64_t Arg1, uint64_t Arg2);<br>
>>>>><br>
>>>>> static ExprOperand getDeref() {<br>
>>>>> return ExprOperand(dwarf::DW_OP_<u></u>deref);<br>
>>>>> }<br>
>>>>> static ExprOperand getPiece(uint64_t A1, uint64_t A2) {<br>
>>>>> return ExprOperand(dwarf::DW_OP_bit_<u></u>piece, A1, A2);<br>
>>>>> }<br>
>>>>> };<br>
>>>>><br>
>>>>> ExprOperand Ops = {<br>
>>>>> ExprOperand::getDeref(),<br>
>>>>> ExprOperand::getPiece(0, 8)<br>
>>>>> };<br>
>>>>> auto *Expr = MDExpression::get(Ops);<br>
>>>>><br>
>>>>> Then the above could trivially be pretty-printed/parsed as:<br>
>>>>><br>
>>>>> !MDExpression(DW_OP_deref, DW_OP_bit_piece(0, 8))<br>
>>>><br>
>>>> or even better ... DW_OP_bit_piece(offset: 0, size: 8)<br>
>>><br>
>>> Sure.<br>
>>><br>
>>>> Yeah that looks good. Also we would need to introduce DW_OP_constu and get rid of the pseudo argument that DW_OP_plus currently takes.<br>
>>><br>
>>> Not necessary. You can represent this already with:<br>
>>><br>
>>> DW_OP_plus(7)<br>
>>><br>
>>> where `7` is the pseudo-argument. This is somewhat horrible, but<br>
>>> so is `DW_OP_plus` taking a pseudo-argument ;).<br>
>>><br>
>>>> This doesn’t need to happen right away, but we should keep it in mind.<br>
>>><br>
>>> I think something like this would simplify the logic a lot, besides<br>
>>> making it more extensible.<br>
>>><br>
>>>>><br>
>>>>> Thoughts?<br>
>>>>><br>
>>>>><br>
>>>>>><br>
>>>>>> Will it still print the old-style comments that testcases tend to match on, or are they obsoleted by the human-readable syntax?<br>
>>>>><br>
>>>>> They're obsoleted. The old-style comments will get stale and just<br>
>>>>> add noise. If we've regressed somehow let me know how.<br>
>>>><br>
>>>> See my comment on DW_OP_bit_piece above :-)<br>
>>>><br>
>>><br>
>>> Hah, right, right :).<br>
>>><br>
>>> I'll see if I can get something together for you. If not we can keep<br>
>>> the DW_TAG_expression comments temporarily.<br>
>>><br>
>>> Is it alright to lose the "offset=" and "size=" markers (at least<br>
>>> temporarily)? I don't see a natural place to put them in the syntax.<br>
>>> E.g., I was thinking I'd aim for the following for now:<br>
>>><br>
>>> !MDExpression(DW_OP_deref, DW_OP_plus, 8, DW_OP_bit_piece, 0, 8)<br>
>>><br>
>>> (and with no comment). Is this sufficient?<br>
>><br>
>> Sufficient as an intermediate step on our way to the above expression being written as<br>
>><br>
>> !MDExpression(DW_OP_deref, DW_OP_constu(8), DW_OP_plus, DW_OP_bit_piece(offset: 0, size: 8))<br>
><br>
> Yup, although I'm happy for someone else to drive that -- the biggest<br>
> change will be switching to `DW_OP_constu`. Once that's done I can<br>
> do the assembly if you want.<br>
><br>
> Attached a new set of patches (included the full series for reference).<br>
> The ones you probably want to look at are:<br>
><br>
> - 0001/0002: getOperationEncoding().<br>
> - 0003: Porting the DIExpression logic over. I did a few things<br>
> differently since the base iterator is random access.<br>
> - 0021: MDExpression.<br>
<br>
All 4 look good to me.<br>
I hope you’ll be able to guide me trough the DW_OP_constu change later?<br>
<br>
We should add a new coding guideline: If you touch llvm.vim you must also update llvm-mode.el and vice versa ;-)<br>
<br>
-- adrian<br>
<br>
><br>
> I'll start pushing through the others while you take a look at the<br>
> changes.<br>
><br>
> <all.patch><br>
><br>
> <0001-Support-Rewrite-<u></u>LocationAtom-and-<u></u>OperationEncodingSt.patch><<u></u>0002-Support-Add-dwarf-<u></u>getOperationEncoding.patch><<u></u>0003-IR-Add-MDExpression-<u></u>ExprOperand.patch><0004-<u></u>AsmWriter-Bitcode-MDSubrange.<u></u>patch><0005-AsmWriter-Bitcode-<u></u>MDEnumerator.patch><0006-<u></u>AsmWriter-Bitcode-MDBasicType.<u></u>patch><0007-AsmWriter-<u></u>MDBasicType-Recognize-DW_ATE-<u></u>in-encoding.patch><0008-<u></u>AsmWriter-Bitcode-MDFile.<u></u>patch><0009-AsmWriter-Bitcode-<u></u>MDDerivedType-and-<u></u>MDCompositeType.patch><0010-<u></u>AsmWriter-MDCompositeType-<u></u>Recognize-DW_LANG-in-runti.<u></u>patch><0011-AsmWriter-Bitcode-<u></u>MDSubroutineType.patch><0012-<u></u>AsmWriter-Bitcode-<u></u>MDCompileUnit.patch><0013-<u></u>AsmWriter-Bitcode-<u></u>MDSubprogram.patch><0014-<u></u>AsmWriter-MDSubprogram-<u></u>Recognize-DW_VIRTUALITY-in-vi.<u></u>patch><0015-AsmWriter-Bitcode-<u></u>MDLexicalBlock.patch><0016-<u></u>AsmWriter-Bitcode-<u></u>MDLexicalBlockFile.patch><<u></u>0017-AsmWriter-Bitcode-<u></u>MDNamespace.patch><0018-<u></u>AsmWriter-Bitcode-MDTemplate-<u></u>Type-Value-Parameter.patch><<u></u>0019-AsmWriter-Bitcode-<u></u>MDGlobalVariable.patch><0020-<u></u>AsmWriter-Bitcode-<u></u>MDLocalVariable.patch><0021-<u></u>AsmWriter-Bitcode-<u></u>MDExpression.patch><0022-<u></u>AsmWriter-Bitcode-<u></u>MDObjCProperty.patch><0023-<u></u>AsmWriter-Bitcode-<u></u>MDImportedEntity.patch><br>
<br>
</blockquote></div>