[llvm] r215384 - Debug Info: Move the sorting and uniqueing of pieces from emitLocPieces()

[David Blaikie]

On Mon, Aug 11, 2014 at 4:33 PM, Adrian Prantl <aprantl at apple.com> wrote:
>
>> On Aug 11, 2014, at 2:34 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>
>> On Mon, Aug 11, 2014 at 2:05 PM, Adrian Prantl <aprantl at apple.com> wrote:
>>> Author: adrian
>>> Date: Mon Aug 11 16:05:55 2014
>>> New Revision: 215384
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=215384&view=rev
>>> Log:
>>> Debug Info: Move the sorting and uniqueing of pieces from emitLocPieces()
>>> into buildLocationList(). By keeping the list of Values sorted,
>>> DebugLocEntry::Merge can also merge multi-piece entries.
>>
>> Is this last sentence a description of new functionality that could be
>
> Yes indeed it is. I’m still searching for a good testcase to reliably reproduce this though. It would certainly be good to test this.
>> tested - or just "this merging can now happen here rather than where
>> it was previously happening (somewhere else)"?
>>
>>>
>>> Modified:
>>>    llvm/trunk/lib/CodeGen/AsmPrinter/DebugLocEntry.h
>>>    llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
>>>
>>> Modified: llvm/trunk/lib/CodeGen/AsmPrinter/DebugLocEntry.h
>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/AsmPrinter/DebugLocEntry.h?rev=215384&r1=215383&r2=215384&view=diff
>>> ==============================================================================
>>> --- llvm/trunk/lib/CodeGen/AsmPrinter/DebugLocEntry.h (original)
>>> +++ llvm/trunk/lib/CodeGen/AsmPrinter/DebugLocEntry.h Mon Aug 11 16:05:55 2014
>>> @@ -76,6 +76,13 @@ public:
>>>       llvm_unreachable("unhandled EntryKind");
>>>     }
>>>
>>> +    // Compare two pieces based on their offset.
>>> +    bool operator<(const Value &other) const {
>>
>> Vague preference for operator overloads as non-members if they can be
>> (the general justification being about implicit conversions being
>> equally valid/desired on LHS and RHS - if it's a member you don't get
>> LHS implicit conversions but you do get RHS implicit conversions...
>> yeah, it's not a /great/ argument, but just a thought), friended if
>> necessary.
>
> Done in 215403.
>>
>>> +      DIVariable Var(Variable);
>>
>> Could we just change Value::Variable's type to DIVariable?
>
> This would make DebugLocEntry::Value loose its POD-likeness.

Is that particularly important?

> I did add a couple of convenience accessors to it though, which should give us most of the same positive effects.

Cool cool,

- David

>
> thanks,
> adrian
>>
>>> +      DIVariable OtherVar(other.Variable);
>>> +      return Var.getPieceOffset() < OtherVar.getPieceOffset();
>>> +    }
>>> +
>>>     bool isLocation() const { return EntryKind == E_Location; }
>>>     bool isInt() const { return EntryKind == E_Integer; }
>>>     bool isConstantFP() const { return EntryKind == E_ConstantFP; }
>>> @@ -87,7 +94,7 @@ public:
>>>     const MDNode *getVariable() const { return Variable; }
>>>   };
>>> private:
>>> -  /// A list of locations/constants belonging to this entry.
>>> +  /// A list of locations/constants belonging to this entry, sorted by offset.
>>>   SmallVector<Value, 1> Values;
>>>
>>> public:
>>> @@ -108,6 +115,7 @@ public:
>>>       if (Var.getName() == NextVar.getName() &&
>>>           Var.isVariablePiece() && NextVar.isVariablePiece()) {
>>>         Values.append(Next.Values.begin(), Next.Values.end());
>>> +        sortUniqueValues();
>>>         End = Next.End;
>>>         return true;
>>>       }
>>> @@ -135,6 +143,14 @@ public:
>>>     assert(DIVariable(Val.Variable).isVariablePiece() &&
>>>            "multi-value DebugLocEntries must be pieces");
>>>     Values.push_back(Val);
>>> +    sortUniqueValues();
>>> +  }
>>> +
>>> +  // Sort the pieces by offset.
>>> +  // Remove any duplicate entries by dropping all but the first.
>>> +  void sortUniqueValues() {
>>> +    std::sort(Values.begin(), Values.end());
>>> +    Values.erase(std::unique(Values.begin(), Values.end()), Values.end());
>>>   }
>>> };
>>>
>>>
>>> Modified: llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp?rev=215384&r1=215383&r2=215384&view=diff
>>> ==============================================================================
>>> --- llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp (original)
>>> +++ llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp Mon Aug 11 16:05:55 2014
>>> @@ -2065,30 +2065,18 @@ void DwarfDebug::emitDebugStr() {
>>> void DwarfDebug::emitLocPieces(ByteStreamer &Streamer,
>>>                                const DITypeIdentifierMap &Map,
>>>                                ArrayRef<DebugLocEntry::Value> Values) {
>>> -  typedef DebugLocEntry::Value Piece;
>>> -  SmallVector<Piece, 4> Pieces(Values.begin(), Values.end());
>>> -  assert(std::all_of(Pieces.begin(), Pieces.end(), [](Piece &P) {
>>> +  assert(std::all_of(Values.begin(), Values.end(), [](DebugLocEntry::Value P) {
>>>         return DIVariable(P.getVariable()).isVariablePiece();
>>>       }) && "all values are expected to be pieces");
>>> -
>>> -  // Sort the pieces so they can be emitted using DW_OP_piece.
>>> -  std::sort(Pieces.begin(), Pieces.end(), [](const Piece &A, const Piece &B) {
>>> -      DIVariable VarA(A.getVariable());
>>> -      DIVariable VarB(B.getVariable());
>>> -      return VarA.getPieceOffset() < VarB.getPieceOffset();
>>> -    });
>>> -  // Remove any duplicate entries by dropping all but the first.
>>> -  Pieces.erase(std::unique(Pieces.begin(), Pieces.end(),
>>> -                           [] (const Piece &A,const Piece &B){
>>> -                             return A.getVariable() == B.getVariable();
>>> -                           }), Pieces.end());
>>> +  assert(std::is_sorted(Values.begin(), Values.end()) &&
>>> +         "pieces are expected to be sorted");
>>>
>>>   unsigned Offset = 0;
>>> -  for (auto Piece : Pieces) {
>>> +  for (auto Piece : Values) {
>>>     DIVariable Var(Piece.getVariable());
>>>     unsigned PieceOffset = Var.getPieceOffset();
>>>     unsigned PieceSize = Var.getPieceSize();
>>> -    assert(Offset <= PieceOffset && "overlapping pieces in DebugLocEntry");
>>> +    assert(Offset <= PieceOffset && "overlapping or duplicate pieces");
>>>     if (Offset < PieceOffset) {
>>>       // The DWARF spec seriously mandates pieces with no locations for gaps.
>>>       Asm->EmitDwarfOpPiece(Streamer, (PieceOffset-Offset)*8);
>>>
>>>
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>