[PATCH] D27361: [DIExpression] Introduce a dedicated DW_OP_LLVM_fragment operation so we can stop using DW_OP_bit_piece with the wrong semantics.

[Paul Robinson via Phabricator via llvm-commits]

probinson added a comment.

Commentary nits.



================
Comment at: include/llvm/IR/DebugInfoMetadata.h:1964
   /// Return the size of this piece in bits.
-  uint64_t getBitPieceSize() const;
+  uint64_t getFragmentSizeInBits() const;
 
----------------
Comment should also s/piece/fragment/?  Here and for isFragment().


================
Comment at: lib/CodeGen/AsmPrinter/DwarfExpression.cpp:116
         // non-zero offset, we need to manually shift the value into
-        // place, since the DW_OP_piece describes the part of the
+        // place, since the DW_OP_fragment describes the part of the
         // variable, not the position of the subregister.
----------------
DW_OP_LLVM_fragment (or DW_OP_piece if it really is still that).


================
Comment at: lib/CodeGen/AsmPrinter/DwarfExpression.cpp:130
   // Keep track of the current position so we can emit the more
-  // efficient DW_OP_piece.
-  unsigned CurPos = PieceOffsetInBits;
+  // efficient DW_OP_fragment.
+  unsigned CurPos = FragmentOffsetInBits;
----------------
DW_OP_LLVM_fragment (or DW_OP_piece if it really is still that).


================
Comment at: lib/CodeGen/AsmPrinter/DwarfExpression.h:158
+  ///                                 fragment inside the entire variable.
+  ///                                 \return false if no DWARF register exists
+  ///                                 for MachineReg.
----------------
The \return line probably should not be indented like this?


================
Comment at: test/DebugInfo/ARM/s-super-register.ll:9
 ; 0x93   DW_OP_piece
-; 0x9d   DW_OP_bit_piece
+; 0x9d   DW_OP_LLVM_fragment
 ; CHECK:            Location description: 90 {{.. .. ((93 ..)|(9d .. ..)) $}}
----------------
0x9d is DW_OP_bit_piece, this comment should not have changed (we're still emitting DW_OP_bit_piece in the actual DWARF, yes?).


Repository:
  rL LLVM

https://reviews.llvm.org/D27361