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

[Adrian Prantl]

> 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. I did add a couple of convenience accessors to it though, which should give us most of the same positive effects.

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