[llvm] r226939 - Move the accessor functions from DIExpression::iterator into a wrapper

David Blaikie dblaikie at gmail.com
Fri Jan 23 14:32:26 PST 2015


On Fri, Jan 23, 2015 at 1:24 PM, Adrian Prantl <aprantl at apple.com> wrote:

> Author: adrian
> Date: Fri Jan 23 15:24:41 2015
> New Revision: 226939
>
> URL: http://llvm.org/viewvc/llvm-project?rev=226939&view=rev
> Log:
> Move the accessor functions  from DIExpression::iterator into a wrapper
> DIExpression::Operand, so we can write range-based for loops.
>
> Thanks to David Blaikie for the idea.
>
> Modified:
>     llvm/trunk/include/llvm/IR/DebugInfo.h
>     llvm/trunk/lib/CodeGen/AsmPrinter/DwarfExpression.cpp
>     llvm/trunk/lib/IR/DebugInfo.cpp
>
> Modified: llvm/trunk/include/llvm/IR/DebugInfo.h
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/IR/DebugInfo.h?rev=226939&r1=226938&r2=226939&view=diff
>
> ==============================================================================
> --- llvm/trunk/include/llvm/IR/DebugInfo.h (original)
> +++ llvm/trunk/include/llvm/IR/DebugInfo.h Fri Jan 23 15:24:41 2015
> @@ -867,16 +867,17 @@ public:
>    /// \brief Return the size of this piece in bytes.
>    uint64_t getPieceSize() const;
>
> +  class Operand;
> +
>    /// \brief An iterator for DIExpression elements.
>    class iterator : public std::iterator<std::forward_iterator_tag,
> StringRef,
>                                          unsigned, const uint64_t *,
> uint64_t> {
>

I'm guessing these types passed to std::iterator need to be updated now
that the value is an Operand, not a uint64_t.


>      DIHeaderFieldIterator I;


Really vaguely curious - should the expression be encoded as a raw byte
sequence, rather than bytes in strings in a DIHeader? (bytes in strings
seems inefficient?)


>      iterator(DIHeaderFieldIterator I) : I(I) {}
> -
>    public:
>      iterator() {}
>      iterator(const DIExpression &Expr) : I(++Expr.header_begin()) {}
> -    uint64_t operator*() const { return I.getNumber<uint64_t>(); }
> +    Operand operator*() const { return Operand(I); }
>

I /think/ you might need to return by reference than by value here (& have
an Operand value as a member of the iterator) - I think that the iterator
requirements require a reference to be returned or some other feature that
makes returning by value not viable - but I could totally be wrong here, my
memory is very vague.


>      iterator &operator++() {
>        increment();
>        return *this;
> @@ -889,14 +890,7 @@ public:
>      bool operator==(const iterator &X) const { return I == X.I; }
>      bool operator!=(const iterator &X) const { return !(*this == X); }
>
> -    uint64_t getArg(unsigned N) const {
> -      auto In = I;
> -      std::advance(In, N);
> -      return In.getNumber<uint64_t>();
> -    }
> -
>      const DIHeaderFieldIterator &getBase() const { return I; }
> -
>    private:
>      void increment() {
>        switch (**this) {
> @@ -911,6 +905,22 @@ public:
>
>    iterator begin() const;
>    iterator end() const;
> +
> +  /// \brief A lightweight wrapper around an element of a DIExpression.
> +  class Operand {
> +    DIHeaderFieldIterator I;
> +  public:
> +    Operand(DIHeaderFieldIterator I) : I(I) {}
>

should this be private & friendedly accessible from the expr iterator?


> +    /// \brief Operands such as DW_OP_piece have explicit (non-stack)
> arguments.
> +    /// Argument 0 is the operand itself.
> +    uint64_t getArg(unsigned N) const {
> +      DIHeaderFieldIterator In = I;
> +      std::advance(In, N);
> +      return In.getNumber<uint64_t>();
> +    }
> +
> +    operator uint64_t () const { return I.getNumber<uint64_t>(); }
>

Maybe this should be an accessor with a name (& type safety?) rather than
an implicit conversion?


> +  };
>  };
>
>  /// \brief This object holds location information.
>
> Modified: llvm/trunk/lib/CodeGen/AsmPrinter/DwarfExpression.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/AsmPrinter/DwarfExpression.cpp?rev=226939&r1=226938&r2=226939&view=diff
>
> ==============================================================================
> --- llvm/trunk/lib/CodeGen/AsmPrinter/DwarfExpression.cpp (original)
> +++ llvm/trunk/lib/CodeGen/AsmPrinter/DwarfExpression.cpp Fri Jan 23
> 15:24:41 2015
> @@ -210,8 +210,8 @@ bool DwarfExpression::AddMachineRegExpre
>    switch (*I) {
>

Can the loop this is within, be turend into a range-based-for loop so
everyone doesn't have to do dereference the iterator.

If not, at least all the "(*I)." below could change to "I->" instead?


>    case dwarf::DW_OP_piece: {
>      unsigned SizeOfByte = 8;
> -    unsigned OffsetInBits = I.getArg(1) * SizeOfByte;
> -    unsigned SizeInBits   = I.getArg(2) * SizeOfByte;
> +    unsigned OffsetInBits = (*I).getArg(1) * SizeOfByte;
> +    unsigned SizeInBits   = (*I).getArg(2) * SizeOfByte;
>      // Piece always comes at the end of the expression.
>      return AddMachineRegPiece(MachineReg, SizeInBits,
>                 getOffsetOrZero(OffsetInBits, PieceOffsetInBits));
> @@ -219,7 +219,7 @@ bool DwarfExpression::AddMachineRegExpre
>    case dwarf::DW_OP_plus:
>      // [DW_OP_reg,Offset,DW_OP_plus,DW_OP_deref] --> [DW_OP_breg,Offset].
>      if (*std::next(I) == dwarf::DW_OP_deref) {
> -      unsigned Offset = I.getArg(1);
> +      unsigned Offset = (*I).getArg(1);
>        ValidReg = AddMachineRegIndirect(MachineReg, Offset);
>        std::advance(I, 2);
>        break;
> @@ -248,14 +248,14 @@ void DwarfExpression::AddExpression(DIEx
>      switch (*I) {
>      case dwarf::DW_OP_piece: {
>        unsigned SizeOfByte = 8;
> -      unsigned OffsetInBits = I.getArg(1) * SizeOfByte;
> -      unsigned SizeInBits   = I.getArg(2) * SizeOfByte;
> +      unsigned OffsetInBits = (*I).getArg(1) * SizeOfByte;
> +      unsigned SizeInBits   = (*I).getArg(2) * SizeOfByte;
>        AddOpPiece(SizeInBits, getOffsetOrZero(OffsetInBits,
> PieceOffsetInBits));
>        break;
>      }
>      case dwarf::DW_OP_plus:
>        EmitOp(dwarf::DW_OP_plus_uconst);
> -      EmitUnsigned(I.getArg(1));
> +      EmitUnsigned((*I).getArg(1));
>        break;
>      case dwarf::DW_OP_deref:
>        EmitOp(dwarf::DW_OP_deref);
>
> Modified: llvm/trunk/lib/IR/DebugInfo.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/DebugInfo.cpp?rev=226939&r1=226938&r2=226939&view=diff
>
> ==============================================================================
> --- llvm/trunk/lib/IR/DebugInfo.cpp (original)
> +++ llvm/trunk/lib/IR/DebugInfo.cpp Fri Jan 23 15:24:41 2015
> @@ -1406,16 +1406,15 @@ void DIVariable::printInternal(raw_ostre
>  }
>
>  void DIExpression::printInternal(raw_ostream &OS) const {
> -  for (auto E = end(), I = begin(); I != E; ++I) {
> -    uint64_t OpCode = *I;
> -    OS << " [" << OperationEncodingString(OpCode);
> -    switch (OpCode) {
> +  for (auto Op : *this) {
> +    OS << " [" << OperationEncodingString(Op);
> +    switch (Op) {
>      case DW_OP_plus: {
> -      OS << " " << I.getArg(1);
> +      OS << " " << Op.getArg(1);
>        break;
>      }
>      case DW_OP_piece: {
> -      OS << " offset=" << I.getArg(1) << ", size=" << I.getArg(2);
> +      OS << " offset=" << Op.getArg(1) << ", size=" << Op.getArg(2);
>        break;
>      }
>      case DW_OP_deref:
>
>
> _______________________________________________
> 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/20150123/26ead4ba/attachment.html>


More information about the llvm-commits mailing list