[Local] replaceAllDbgUsesWith: Update debug values before RAUW

[vsk]

Summary

The replaceAllDbgUsesWith utility helps passes preserve debug info when
replacing one value with another.

This improves upon the existing insertReplacementDbgValues API by:

• Updating debug intrinsics in-place, while preventing use-before-def of the replacement value.
• Falling back to salvageDebugInfo when a replacement can't be made.
• Moving the responsibiliy for rewriting llvm.dbg.* DIExpressions into common utility code.

Along with the API change, this teaches replaceAllDbgUsesWith how to
create DIExpressions for three basic integer and pointer conversions:

• The no-op conversion. Applies when the values have the same width, or have bit-for-bit compatible pointer representations.
• Truncation. Applies when the new value is wider than the old one.
• Zero/sign extension. Applies when the new value is narrower than the old one.

Testing:

• check-llvm, check-clang, a stage2 -g -O3 build of clang, regression/unit testing.
• This resolves a number of mis-sized dbg.value diagnostics from Debugify.

[aprantl]

include/llvm/IR/DebugInfoMetadata.h

785

Why not make an enum { Unknown, Signed, Unsigned } ?

test/Transforms/InstCombine/cast-set-preserve-signed-dbg-val.ll

17

wouldn't DW_OP_lit0 DW_OP_not be shorter than DW_OP_constu 0xfffffff ?

[aprantl]

test/Transforms/InstCombine/cast-set-preserve-signed-dbg-val.ll

17

To keep the IR simple we could also recognize and normalize this sequence in DwarfExpression.cpp

[vsk]

I've incorporated some offline review feedback from @aprantl. We can assume that a debugger will set the high bits of a value to 0, so we can make zero-extension a no-op.

include/llvm/IR/DebugInfoMetadata.h

785

It makes it a bit too easy to make this mistake:

bool IsUnsigned = T.getSignedness() != Signedness::Signed;

test/Transforms/InstCombine/cast-set-preserve-signed-dbg-val.ll

17

Sounds good, thanks! I'll save the normalizing change for later.

[vsk]

I'm double-checking the DWARF for sign extension. Here is what I see in the stage2 llc binary:

0x00754923:                     DW_TAG_inlined_subroutine
                                  DW_AT_abstract_origin	(0x0000000000751e3a "_ZL28isPotentialBlockedMemCpyPairii")
                                  DW_AT_ranges [... snip]
                                  DW_AT_call_file	("/Users/vsk/src/llvm.org-master/llvm/lib/Target/X86/X86AvoidStoreForwardingBlocks.cpp")
                                  DW_AT_call_line	(546)

0x00754930:                       DW_TAG_formal_parameter
                                    DW_AT_location	(0x00186529
                                       [0x00000000000008a4,  0x00000000000008bb): DW_OP_reg2 RCX, DW_OP_piece 0x2, DW_OP_breg2 RCX+0, DW_OP_constu 0xffff, DW_OP_and, DW_OP_constu 0xf, DW_OP_shr, DW_OP_lit0, DW_OP_not, DW_OP_mul, DW_OP_stack_value, DW_OP_piece 0x2
                                       [0x00000000000008fe,  0x0000000000000906): DW_OP_reg2 RCX, DW_OP_piece 0x2, DW_OP_breg2 RCX+0, DW_OP_constu 0xffff, DW_OP_and, DW_OP_constu 0xf, DW_OP_shr, DW_OP_lit0, DW_OP_not, DW_OP_mul, DW_OP_stack_value, DW_OP_piece 0x2
                                       [0x000000000000096e,  0x0000000000000971): DW_OP_reg2 RCX, DW_OP_piece 0x2, DW_OP_breg2 RCX+0, DW_OP_constu 0xffff, DW_OP_and, DW_OP_constu 0xf, DW_OP_shr, DW_OP_lit0, DW_OP_not, DW_OP_mul, DW_OP_stack_value, DW_OP_piece 0x2)
                                    DW_AT_abstract_origin	(0x0000000000751e4b "LdOpcode")

This shows that the DIExpression is lowered correctly, I think. The variable "LdOpcode" is an "int" in the source, but some optimization has us using only its lower 16 bits. Unfortunately, lldb doesn't accept this variable description:

~/src/builds/llvm.org-master-RA-stage2 (0) $ lldb -- ./bin/llc /Users/vsk/src/llvm.org-master/llvm/test/CodeGen/X86/avoid-sfb-overlaps.ll -mtriple=x86_64-linux -o -
(lldb) b isPotentialBlockedMemCpyPair
Breakpoint 1: where = llc`(anonymous namespace)::X86AvoidSFBPass::runOnMachineFunction(llvm::MachineFunction&) + 1188 [inlined] isPotentialBlockedMemCpyPair(int, int) at X86AvoidStoreForwardingBlocks.cpp:546, address = 0x0000000100078cb4
...
Process 24842 stopped
* thread #1, queue = 'com.apple.main-thread', stop reason = breakpoint 1.1
    frame #0: 0x0000000100078cb4 llc`(anonymous namespace)::X86AvoidSFBPass::runOnMachineFunction(llvm::MachineFunction&) [inlined] isPotentialBlockedMemCpyPair(LdOpcode=<unavailable>, StOpcode=<unavailable>) at X86AvoidStoreForwardingBlocks.cpp:162 [opt]
   159  }
   160
   161  static bool isPotentialBlockedMemCpyPair(int LdOpcode, int StOpcode) {
-> 162    switch (LdOpcode) {
 
(lldb) p LdOpcode
error: Couldn't materialize: couldn't get the value of variable LdOpcode: DW_OP_piece for offset 2 but top of stack is of size 8
error: errored out in DoExecute, couldn't PrepareToExecuteJITExpression

Here is what the DWARF4 standard says:

The DW_OP_piece operation takes a single operand, which is an unsigned LEB128 number. The number describes the size in bytes of the piece of the object referenced by the preceding simple location description. If the piece is located in a register, but does not occupy the entire register, the placement of the piece within that register is defined by the ABI.

With this patch, LLVM emits DW_OP_reg2 RCX, DW_OP_piece 0x2. LLDB should know to interpret this as the low 16 bits of %rcx. We'll have to teach it.

[aprantl]

test/Transforms/InstCombine/cast-set-preserve-signed-dbg-val.ll

17

While you're at it:

DW_OP_constu, 15 could be shortened to DW_OP_lit15 :-)

It should also be more efficient to join the two fragments <lower> <upper> DW_OP_lit16 DW_OP_shl DW_OP_or
That saves two DW_OP_LLVM_fragment expressions and one dbg.value.

[vsk]

test/Transforms/InstCombine/cast-set-preserve-signed-dbg-val.ll

17

Nice, I've made a note of the DW_OP_litX (X < 32) normalization :).

I don't see how to use a single dbg.value to describe a sign-extended variable. You need to refer to the value operand of the dbg.value twice: once for the initial SHR, and again at the end for the OR. Are you suggesting making this work using DW_OP_dup? So, for this example:

DW_OP_dup /* Duplicate the 16-bit and */, lit15, SHR, lit0, NOT, MUL /* Calculate the high bits */, OR /* Combine the high bits with the duplicated low bits */?

[vsk]

In this update I've taken @aprantl's suggestion and used DW_OP_dup to get the sign-extension down to one dbg.value. This is a neat simplification. One benefit is that LLDB actually understands DW_OP_dup :). The failing "LdOpcode" example now works:

    frame #0: 0x0000000100077f24 llc`(anonymous namespace)::X86AvoidSFBPass::runOnMachineFunction(llvm::MachineFunction&) [inlined] isPotentialBlockedMemCpyPair(LdOpcode=1899, StOpcode=1898) at X86AvoidStoreForwardingBlocks.cpp:162 [opt]
   159  }
   160 
   161  static bool isPotentialBlockedMemCpyPair(int LdOpcode, int StOpcode) {
-> 162    switch (LdOpcode) {
   163    case X86::MOVUPSrm:
   164    case X86::MOVAPSrm:
   165      return StOpcode == X86::MOVUPSmr || StOpcode == X86::MOVAPSmr;
Target 0: (llc) stopped.
(lldb) p LdOpcode
(int) $0 = 1899

[vsk]

• Remove some defensive code meant to deal with failed fragment creation. This patch doesn't rely on creating fragments any more.

[bjope]

lib/Transforms/Utils/Local.cpp

1756

Not sure that I understand this. You say that if at least one of the types is a pointer, and neither is a non-integral pointer type, then they are compatible.
Don't you need to check that the sizes match here? Or in what sense are they compatible?

1821

Some day I think we need to implement support for the types expression stack from DWARF 5. Currently I think LLVM assumes that the DWARF expression stack is 64 bits wide (or in LLVM IR we maybe assume that we have infinite bits and can do DW_OP_shr etc on any type).

For our target the expression stack is 32 bits. And when legalizing larger types like i64 at ISel we end up trying to split these dbg.intrinsics into several DBG_VALUE instruction, using DW_OP_LLVM_fragment for to identify the different parts. Right now we have no simple way in rewriting a complex DIExpression (using 64-bit expressions) into smaller pieces, so we need to discard the debug info.

1826

I don't think WithStackValue is correct if there could be users from findDbgUsers that is a dbg.declare (because dbg.declare is indirect).
Maybe unlikely to happen here, but I don't know. The same kind of fault exist in salvageDebugInfo, and I've been trying to understand if we ever expect to have non-empty DIExpressions in dbg.declare (except for DW_OP_LLVM_fragment). For example having a DW_OP_stack_value in dbg.declare seems weird, right?

And what happens if we have a dbg.value and the DIExpression already got a non-empty DIExpression, ending without a DW_OP_stack_value. Then it is indirect, and adding DW_OP_stack_value would turn it into being direct. Or should it perhaps be illegal to have an indirect dbg.value like that?

[aprantl]

test/Transforms/InstCombine/cast-set-preserve-signed-dbg-val.ll

17

You are correct, DW_OP_dup exists exactly for this kind of use-case.

[vsk]

lib/Transforms/Utils/Local.cpp

1756

I'd like this to be a target-aware way to identify semantically-equivalent bitcasts, inttoptr, and ptrtoint conversions. I'm using isNonIntegralPointerType because, AFAIK, inttoptr/ptrtoint are not lossy on targets with integral pointers. I assumed that the sizes of FromTy and ToTy are equal when at least one of the types is a pointer type, but I'm no longer sure this is a valid assumption. I'll rewrite this to be cleaner and add an explicit check.

1821

I think LLVM does assume a 64-bit width for the expression stack (at least, we can't represent any DW_OP_constu wider than 64-bits in the IR).

The scheme you describe of splitting up the dbg intrinsics for i64 values and rewriting their expressions sounds very difficult. I'm not familiar with any backends, but IIRC legalization occurs early, so a split i32 may be optimized out leaving you with an incomplete (actually, incorrect) variable description? Does DWARF5 alleviate the issue at all? Maybe you could introduce a vendor extension to apply DW_OP_convert to a subset of a source variable's bits?

1826

You're correct to point out that the operand of a dbg.{addr,declare} is the address of a variable, not the variable itself. But I don't see how we'd take the sign-extension code path here when the dbg intrinsic operand is a pointer. Actually, allowing this utility and salvageDI to operate on dbg.addr seems useful, because they can preserve debug info across no-op pointer bitcasts.

I don't think we have any code that would add a DW_OP_stack_value to a dbg.{addr, declare}, but if we did, I don't think it would be weird. IIUC, it would just mean the debugger needs to do pointer arithmetic before loading a variable. That seems just like having a dbg.value for a pointer operand and adding a DW_OP_deref at the end (before DW_OP_stack_value).

I don't understand your last question about dbg.values with non-empty, non-stack-value expressions. It seems reasonable to create these (say, by using DW_OP_deref), and I don't see why they can't also be treated as stack values.

[dblaikie]

Side note: please don't depend on what type a pointer points to, if
possible (& it should be possible everywhere except where necessary to
match the IR constraints - eg: you may need to check a pointer's pointee
type to ensure a use of a value of that type matches up with its
definition). Eventually pointee types will go away & someone will have to
fix up that code... best to avoid that if we can.

[bjope]

lib/Transforms/Utils/Local.cpp

1826

I don't understand your last question about dbg.values with non-empty, non-stack-value expressions. It seems reasonable to create these (say, by using DW_OP_deref), and I don't see why they can't also be treated as stack values.

Let's say that we have

dbg.value  i32 %x, !y, DIExpression(DW_OP_lit1, DW_OP_add)

then it means that the value of !y is given by dereferencing (%x+1).
If %x can be rewritten as a sign-extend of some value %z, and you use

prependOpcodes(OldDII.getExpression(), Ops, DIExpression::WithStackValue);

then, afaik, the DW_OP_stack_value is added at the end of the expression, while the Ops are prepended. So you end up with

dbg.value  i32 %z, !y, DIExpression(Ops, DW_OP_lit1, DW_OP_add, DW_OP_stack_value)

So we will end up saying that !y is equal to (%x + 1) instead of that (%x + 1) points to the value.

This could be an unexpected situation here, never supposed to happen. Then some asserts showing those assumptions might be good. Maybe such asserts could be put inside prependOpcodes.

[vsk]

lib/Transforms/Utils/Local.cpp

1826

Now I understand, thanks. The key language from the docs I was missing is:
"DIExpressions also follow this model: A DIExpression that doesn’t have a trailing DW_OP_stack_value will describe an address when combined with a concrete location."

This means the current patch does not handle any dbg.{addr,declare} or non-empty DIExpressions properly. It would mess up the variable's address calculation and/or pre-existing stack_value computations.

I think we should handle this case. I'll try to find a way to normalize expressions as stack values before appending any DW_OPs to them.

[vsk]

• Introduce and use DIExpression::appendToStack() to create sign extension stack values. It takes care of dereferencing indirect values when needed.
• Rename insertReplacementDbgValues to replaceAllDbgUsesWith. The main change is that debug intrinsics are now updated in-place. The previous behavior of inserting new debug values could reorder the way variable updates are seen in the debugger.

[gramanas]

lib/Transforms/Utils/Local.cpp

1803

Could FromBits be equal to ToBits? In that case neither zext nor sext is needed. Could this be if (FromBits <= ToBits)?

[aprantl]

include/llvm/IR/DebugInfoMetadata.h

2332

This looks very dangerous. DIExpressions are not distinct so they may be shared by many users. Shouldn't we create a new DIExpression instead of modifying the old one?

[vsk]

include/llvm/IR/DebugInfoMetadata.h

2332

Why does this modify an existing expression? I think I'm only using it to create a new expression (in DIExpression::appendToStack, appendToVector(NewOps)).

lib/Transforms/Utils/Local.cpp

1803

No, that case should be handled earlier, in isBitCastSemanticsPreserving().

[aprantl]

include/llvm/IR/DebugInfoMetadata.h

2332

My bad. I overlooked that this is a const member and that V is the one being modified. Sorry for the noise!

[bjope]

FYI: Looks like you have address my earlier comments. So I have no blocking remarks.

The addition of DomPoint / DT was interesting (not sure that I have figured out exactly which scenarios that are handled)
I think that we still have the problem with only checking dominance of the moved/replaced value though.
As was discussed earlier somewhere (a patch in post RA MachineSink) we also might have the problem that there are other dbg.value intrinsics, referring to the same variable (or an overlapping fragment). And the question is if it is correct to reorder such dbg.value intrinsics, or what to do. Matt wrote a TR about it here https://bugs.llvm.org/show_bug.cgi?id=37897

lib/Transforms/Utils/Local.cpp

1803

It would still be more correct to enter the if in case FromBits==ToBits. So I don't think it hurts to use <= here.

[vsk]

• Rebase.
• Address the FromBits == ToBits case.
• Extend the unit test. There are two new values, %f and %g, which are used to demonstrate what happens in the two use-before-def scenarios: either we're able to salvage the debug user, or we need to delete it.

[aprantl]

lib/Transforms/Utils/Local.cpp

1812

There is no way to sign-extend, that's right, but zero-extension as a concept doesn't make much sense in the untyped DWARF (4) stack, since we don't know how large the stack values are anyway. Looks like the code knows this but the comment hasn't been updated yet.

[aprantl]

Thanks, I think this looks good now!

[vsk]

In D48676#1153467, @bjope wrote:

FYI: Looks like you have address my earlier comments. So I have no blocking remarks.

Thanks for taking a look, I really appreciate your detailed feedback.

The addition of DomPoint / DT was interesting (not sure that I have figured out exactly which scenarios that are handled)

The two scenarios are:

1. The replacement value is a Constant, will be inserted at a position which dominates the original instruction, or will be inserted immediately after the original instruction (which is deleted later). In this scenario there's no risk of introducing debug value use-before-def.
2. The original value dominates the position where the replacement value will be inserted. A good example of this is the 'alloca-cast-debuginfo.ll' test. In an intermediate step, InstCombine inserts a temporary cast of an alloca:

%local = alloca %struct.Foo
%tmpcast = bitcast %local
call dbg.declare(%tmpcast, "local")
<some instructions follow>
%castOfTmpcast = bitcast %tmpcast

InstCombine recognizes that it can remove 'tmpcast' and simplify 'castOfTmpcast' if it promotes the type of the alloca. Since 'tmpcast' has only one use, we try to save its debug users, anticipating that 'tmpcast' will be deleted. The DomPoint / DT machinery gives us a way to prevent this use-before-def for the unlinked 'simplifiedCast':

%local = alloca i64
call dbg.declare(%simplifiedCast, "local")
<some instructions follow>
%simplifiedCast = bitcast %local

We either salvage the debug user or delete it. The unit test has examples of both situations.

I think that we still have the problem with only checking dominance of the moved/replaced value though.
As was discussed earlier somewhere (a patch in post RA MachineSink) we also might have the problem that there are other dbg.value intrinsics, referring to the same variable (or an overlapping fragment). And the question is if it is correct to reorder such dbg.value intrinsics, or what to do. Matt wrote a TR about it here https://bugs.llvm.org/show_bug.cgi?id=37897

Doesn't this patch sidestep issues with reordering variable updates, because the updates are now all done in-place?

[bjope]

In D48676#1153642, @vsk wrote:

In D48676#1153467, @bjope wrote:

FYI: Looks like you have address my earlier comments. So I have no blocking remarks.

Thanks for taking a look, I really appreciate your detailed feedback.

The addition of DomPoint / DT was interesting (not sure that I have figured out exactly which scenarios that are handled)

The two scenarios are:

1. The replacement value is a Constant, will be inserted at a position which dominates the original instruction, or will be inserted immediately after the original instruction (which is deleted later). In this scenario there's no risk of introducing debug value use-before-def.
2. The original value dominates the position where the replacement value will be inserted. A good example of this is the 'alloca-cast-debuginfo.ll' test. In an intermediate step, InstCombine inserts a temporary cast of an alloca:

%local = alloca %struct.Foo
%tmpcast = bitcast %local
call dbg.declare(%tmpcast, "local")
<some instructions follow>
%castOfTmpcast = bitcast %tmpcast

InstCombine recognizes that it can remove 'tmpcast' and simplify 'castOfTmpcast' if it promotes the type of the alloca. Since 'tmpcast' has only one use, we try to save its debug users, anticipating that 'tmpcast' will be deleted. The DomPoint / DT machinery gives us a way to prevent this use-before-def for the unlinked 'simplifiedCast':

%local = alloca i64
call dbg.declare(%simplifiedCast, "local")
<some instructions follow>
%simplifiedCast = bitcast %local

We either salvage the debug user or delete it. The unit test has examples of both situations.

I think that we still have the problem with only checking dominance of the moved/replaced value though.
As was discussed earlier somewhere (a patch in post RA MachineSink) we also might have the problem that there are other dbg.value intrinsics, referring to the same variable (or an overlapping fragment). And the question is if it is correct to reorder such dbg.value intrinsics, or what to do. Matt wrote a TR about it here https://bugs.llvm.org/show_bug.cgi?id=37897

Doesn't this patch sidestep issues with reordering variable updates, because the updates are now all done in-place?

I thought that

DII->moveAfter(&DomPoint);

still could move the DII without really taking other dbg.value intrinsics, potentially describing the same variable, into account.
But I see now that the moveAfter only happens when

DomPointAfterFrom && DII->getNextNonDebugInstruction() == &DomPoint

which means that we do not move the DII arbitrarely (it's just moving to the other side of the DomPoint). So it should be ok.

include/llvm/IR/DebugInfoMetadata.h

DEFINE_MDNODE_GET(DIBasicType,
                     (unsigned Tag, MDString *Name, uint64_t SizeInBits,
                      uint32_t AlignInBits, unsigned Encoding),
                     (Tag, Name, SizeInBits, AlignInBits, Encoding))
 
   TempDIBasicType clone() const { return cloneImpl(); }
 
   unsigned getEncoding() const { return Encoding; }
 
+  enum class Signedness { Signed, Unsigned };
+
+  /// Return the signedness of this type, or None if this type is neither
+  /// signed nor unsigned.
+  Optional<Signedness> getSignedness() const;
+
   static bool classof(const Metadata *MD) {
     return MD->getMetadataID() == DIBasicTypeKind;
   }
 };
 
 /// Derived types.
 ///
 /// This includes qualified types, pointers, references, friends, typedefs, and
public:
   StringRef getName() const { return getStringOperand(1); }
   DIFile *getFile() const { return cast_or_null<DIFile>(getRawFile()); }
   DITypeRef getType() const { return DITypeRef(getRawType()); }
   uint32_t getAlignInBits() const { return AlignInBits; }
   uint32_t getAlignInBytes() const { return getAlignInBits() / CHAR_BIT; }
   /// Determines the size of the variable's type.
   Optional<uint64_t> getSizeInBits() const;
 
+  /// Return the signedness of this variable's type, or None if this type is
+  /// neither signed nor unsigned.
+  Optional<DIBasicType::Signedness> getSignedness() const {
+    if (auto *BT = dyn_cast<DIBasicType>(getType().resolve()))
+      return BT->getSignedness();
+    return None;
+  }
+
   StringRef getFilename() const {
     if (auto *F = getFile())
       return F->getFilename();
     return "";
   }
 
   StringRef getDirectory() const {
     if (auto *F = getFile())
public:
 
     /// Return the size of the operand.
     ///
     /// Return the number of elements in the operand (1 + args).
     unsigned getSize() const;
   };
 
   /// An iterator for expression operands.
   class expr_op_iterator
       : public std::iterator<std::input_iterator_tag, ExprOperand> {
     ExprOperand Op;
 
   public:
     expr_op_iterator() = default;
     explicit expr_op_iterator(element_iterator I) : Op(I) {}
 
     element_iterator getBase() const { return Op.get(); }

include/llvm/Transforms/Utils/Local.h

 /// Assuming the instruction \p I is going to be deleted, attempt to salvage any
 /// dbg.value intrinsics referring to \p I by rewriting its effect into a
 /// DIExpression.
 void salvageDebugInfo(Instruction &I);
 
 /// Assuming the value \p From is going to be deleted, insert replacement
 /// dbg.value intrinsics for each debug user of \p From. The newly-inserted
 /// dbg.values refer to \p To instead of \p From. Each replacement dbg.value
-/// has the same location and variable as the debug user it replaces, has a
-/// DIExpression determined by the result of \p RewriteExpr applied to an old
-/// debug user of \p From, and is placed before \p InsertBefore. If
-/// \p RewriteExpr returns nullptr, no replacement for the specified debug
-/// user is emitted.
-void insertReplacementDbgValues(
-    Value &From, Value &To, Instruction &InsertBefore,
-    function_ref<DIExpression *(DbgInfoIntrinsic &OldDII)> RewriteExpr);
-
-/// An overload of insertReplacementDbgValues() for the common case where
-/// the replacement dbg.values have the same DIExpressions as the originals.
+/// has the same location and variable as the debug user it replaces and is
+/// placed before \p InsertBefore. \p From must be convertible to \p To using
+/// truncation, (sign|zero|fp)-extension, or the identity mapping.
+///
+/// Note that creating replacement dbg.values may fail. This function guarantees
+/// that either a dbg.value replacement will be inserted for every debug user of
+/// \p From, or no replacements will be inserted.
 void insertReplacementDbgValues(Value &From, Value &To,
                                 Instruction &InsertBefore);
 
 /// Remove all instructions from a basic block other than it's terminator
 /// and any present EH pad instructions.
 unsigned removeAllNonTerminatorAndEHPadInstructions(BasicBlock *BB);
 
 /// Insert an unreachable instruction before the specified

lib/IR/DebugInfoMetadata.cpp

DIBasicType *DIBasicType::getImpl(LLVMContext &Context, unsigned Tag,
   assert(isCanonical(Name) && "Expected canonical MDString");
   DEFINE_GETIMPL_LOOKUP(DIBasicType,
                         (Tag, Name, SizeInBits, AlignInBits, Encoding));
   Metadata *Ops[] = {nullptr, nullptr, Name};
   DEFINE_GETIMPL_STORE(DIBasicType, (Tag, SizeInBits, AlignInBits, Encoding),
                        Ops);
 }
 
+Optional<DIBasicType::Signedness> DIBasicType::getSignedness() const {
+  switch (getEncoding()) {
+  case dwarf::DW_ATE_signed:
+  case dwarf::DW_ATE_signed_char:
+    return Signedness::Signed;
+  case dwarf::DW_ATE_unsigned:
+  case dwarf::DW_ATE_unsigned_char:
+    return Signedness::Unsigned;
+  default:
+    return None;
+  }
+}
+
 DIDerivedType *DIDerivedType::getImpl(
     LLVMContext &Context, unsigned Tag, MDString *Name, Metadata *File,
     unsigned Line, Metadata *Scope, Metadata *BaseType, uint64_t SizeInBits,
     uint32_t AlignInBits, uint64_t OffsetInBits,
     Optional<unsigned> DWARFAddressSpace, DIFlags Flags, Metadata *ExtraData,
     StorageType Storage, bool ShouldCreate) {
   assert(isCanonical(Name) && "Expected canonical MDString");
   DEFINE_GETIMPL_LOOKUP(DIDerivedType,

lib/Transforms/InstCombine/InstCombineCasts.cpp

Instruction *InstCombiner::commonCastTransforms(CastInst &CI) {
   if (auto *CSrc = dyn_cast<CastInst>(Src)) {   // A->B->C cast
     if (Instruction::CastOps NewOpc = isEliminableCastPair(CSrc, &CI)) {
       // The first cast (CSrc) is eliminable so we need to fix up or replace
       // the second cast (CI). CSrc will then have a good chance of being dead.
       auto *Ty = CI.getType();
       auto *Res = CastInst::Create(NewOpc, CSrc->getOperand(0), Ty);
       // Replace debug users of the eliminable cast by emitting debug values
       // which refer to the new cast.
-      if (Ty->isIntegerTy() || Ty->isPointerTy())
-        // TODO: Support floats and vectors (see DW_OP_convert, fragment).
-        insertReplacementDbgValues(*CSrc, *Res, *std::next(CI.getIterator()));
       return Res;
     }
   }
 
   if (auto *Sel = dyn_cast<SelectInst>(Src)) {
     // We are casting a select. Try to fold the cast into the select, but only
     // if the select does not have a compare instruction with matching operand
     // types. Creating a select with operands that are different sizes than its

lib/Transforms/Utils/Local.cpp

for (auto *DII : DbgUsers) {
       DIExpr = DIExpression::prepend(DIExpr, DIExpression::WithDeref);
       DII->setOperand(0, AddrMD);
       DII->setOperand(2, MetadataAsValue::get(I.getContext(), DIExpr));
       LLVM_DEBUG(dbgs() << "SALVAGE:  " << *DII << '\n');
     }
   }
 }
 
-void llvm::insertReplacementDbgValues(
-    Value &From, Value &To, Instruction &InsertBefore,
-    function_ref<DIExpression *(DbgInfoIntrinsic &OldDII)> RewriteExpr) {
+/// A dbg.value replacement consists of a replacement value operand and a new
+/// (possibly null) DIExpression.
+using DbgValReplacement = std::pair<Value *, DIExpression *>;
+
+/// Insert replacement dbg.values for \p From. The value operand and
+/// DIExpression for new dbg.values is determined by \p RewriteExpr. A null
+/// DIExpression indicates that no dbg.value should be emitted.
+///
+/// Returns true if \p From has debug users, and all replacement dbg.values for
+/// these users were emitted.
+static bool replaceDbgValues(
+    Value &From, Instruction &InsertBefore,
+    function_ref<DbgValReplacement(DbgInfoIntrinsic &OldDII)> RewriteExpr) {
   // Collect all debug users of From.
   SmallVector<DbgInfoIntrinsic *, 1> Users;
   findDbgUsers(Users, &From);
   if (Users.empty())
-    return;
+    return false;
 
   // Insert a replacement debug value for each old debug user. It's assumed
   // that the old debug users will be erased later.
+  SmallVector<Instruction *, 1> Replacements;
   DIBuilder DIB(*InsertBefore.getModule());
-  for (auto *OldDII : Users)
-    if (DIExpression *Expr = RewriteExpr(*OldDII)) {
-      auto *I = DIB.insertDbgValueIntrinsic(&To, OldDII->getVariable(), Expr,
-                                            OldDII->getDebugLoc().get(),
-                                            &InsertBefore);
-      (void)I;
-      LLVM_DEBUG(dbgs() << "REPLACE:  " << *I << '\n');
-    }
+  for (auto *OldDII : Users) {
+    DbgValReplacement Replacement = RewriteExpr(*OldDII);
+    if (!Replacement.second)
+      break;
+
+    auto *I = DIB.insertDbgValueIntrinsic(
+        Replacement.first, OldDII->getVariable(), Replacement.second,
+        OldDII->getDebugLoc().get(), &InsertBefore);
+    Replacements.push_back(I);
+
+  // Replacements for all users were inserted.
+  if (Replacements.size() == Users.size())
+    return true;
+
+  // Some replacements could not be emitted. Erase any that were just inserted.
+  for (Instruction *I : Replacements) {
+    LLVM_DEBUG(dbgs() << "ERASING INCOMPLETE REPLACEMENT:  " << *I << '\n');
+    I->eraseFromParent();
+  }
+  return false;
+}
+
+/// Check if the debug info representation of values of type \p FromTy and \p
+/// ToTy are compatible.
+///
+/// Note that Type::canLosslesslyBitCastTo is not suitable here because it
+/// allows semantically unequivalent bitcasts, such as <2 x i64> -> <4 x i32>,
+/// but does not allow lossless pointer <-> integer conversions.
+static bool hasCompatibleDebugInfoRepresentation(const DataLayout &DL,
+                                                 Type *FromTy, Type *ToTy) {
+  // Trivially compatible types.
+  if (FromTy == ToTy)
+    return true;
+
+  // Handle compatible pointer <-> integer conversions.
+  return (FromTy->isPointerTy() || ToTy->isPointerTy()) &&
+         !DL.isNonIntegralPointerType(FromTy) &&
+         !DL.isNonIntegralPointerType(ToTy);
 }
 
 void llvm::insertReplacementDbgValues(Value &From, Value &To,
                                       Instruction &InsertBefore) {
-  return llvm::insertReplacementDbgValues(
-      From, To, InsertBefore,
-      [](DbgInfoIntrinsic &OldDII) { return OldDII.getExpression(); });
+  Type *FromTy = From.getType();
+  Type *ToTy = To.getType();
+
+  auto Identity = [&](DbgInfoIntrinsic &OldDII) -> DbgValReplacement {
+    return {&To, OldDII.getExpression()};
+  };
+
+  // The simple case: no conversion between the values is necessary.
+  const DataLayout &DL = InsertBefore.getModule()->getDataLayout();
+  if (hasCompatibleDebugInfoRepresentation(DL, FromTy, ToTy)) {
+    replaceDbgValues(From, InsertBefore, Identity);
+    return;
+  }
+
+  // Handle integer-to-integer widening and narrowing.
+  // FIXME: Use DW_OP_convert when it's available everywhere.
+  if (FromTy->isIntegerTy() && ToTy->isIntegerTy()) {
+    uint64_t FromBits = FromTy->getPrimitiveSizeInBits();
+    uint64_t ToBits = ToTy->getPrimitiveSizeInBits();
+
+    // When the width of the result grows, assume that a debugger will only
+    // access the low `FromBits` bits when inspecting the source variable.
+    if (FromBits < ToBits) {
+      replaceDbgValues(From, InsertBefore, Identity);
+      return;
+    }
+
+    // The width of the result has shrunk. First, do sign/zero extension to
+    // describe the source variable's high bits.
+    auto SignOrZeroExt = [&](DbgInfoIntrinsic &OldDII) -> DbgValReplacement {
+      DILocalVariable *Var = OldDII.getVariable();
+
+      // If the variable has no signedness or has the wrong size, do not
+      // attempt to replace any dbg.values.
+      auto Signedness = Var->getSignedness();
+      auto VarSize = Var->getSizeInBits();
+      if (!Signedness || (!VarSize || *VarSize != FromBits))
+        return {nullptr, nullptr};
+
+      bool Signed = *Signedness == DIBasicType::Signedness::Signed;
+
+      // Sign extension for a >64-bit integer can't be represented yet.
+      if (Signed && FromBits > 64)
+        return {nullptr, nullptr};
+
+      if (!Signed) {
+        // In the unsigned case, the high bits are just 0.
+        return {ConstantInt::get(FromTy, 0),
+                DIExpression::createFragmentExpression(
+                    OldDII.getExpression(), ToBits, FromBits - ToBits)
+                    .getValueOr(nullptr)};
+      } else {
+        // In the signed case, the high bits are given by sign extension, i.e:
+        //   (To >> (ToBits - 1)) * ((2 ^ FromBits) - 1)
+        uint64_t Mask = APInt::getAllOnesValue(FromBits).getLimitedValue();
+        SmallVector<uint64_t, 8> Ops({dwarf::DW_OP_constu, (ToBits - 1),
+                                      dwarf::DW_OP_shr, dwarf::DW_OP_constu,
+                                      Mask, dwarf::DW_OP_mul});
+        auto *Expr = DIExpression::prependOpcodes(OldDII.getExpression(), Ops,
+                                                  DIExpression::WithStackValue);
+        return {&To, DIExpression::createFragmentExpression(Expr, ToBits,
+                                                            FromBits - ToBits)
+                         .getValueOr(nullptr)};
+      }
+    };
+    bool ReplacedAllUsers = replaceDbgValues(From, InsertBefore, SignOrZeroExt);
+    if (!ReplacedAllUsers)
+      return;
+
+    // Finally, describe the low `ToBits` bits of the source variable.
+    ReplacedAllUsers = replaceDbgValues(
+        From, InsertBefore, [&](DbgInfoIntrinsic &OldDII) -> DbgValReplacement {
+          return {&To, DIExpression::createFragmentExpression(
+                           OldDII.getExpression(), 0, ToBits)
+                           .getValueOr(nullptr)};
+        });
+    assert(ReplacedAllUsers && "Could not describe low bits of variable");
+  }
+
+  // TODO: Floating-point conversions, vectors.
 }
 
 unsigned llvm::removeAllNonTerminatorAndEHPadInstructions(BasicBlock *BB) {
   unsigned NumDeadInst = 0;
   // Delete the instructions backwards, as it has a reduced likelihood of
   // having to update as many def-use and use-def chains.
   Instruction *EndInst = BB->getTerminator(); // Last not to be deleted.
   while (EndInst != &BB->front()) {

test/Transforms/InstCombine/cast-set-preserve-signed-dbg-val.ll

+; RUN: opt -instcombine -S < %s | FileCheck %s
+
+; CHECK-LABEL: define {{.*}} @test5
+define i16 @test5(i16 %A) !dbg !34 {
+  ; CHECK: [[and:%.*]] = and i16 %A, 15
+
+  %B = sext i16 %A to i32, !dbg !40
+  call void @llvm.dbg.value(metadata i32 %B, metadata !36, metadata !DIExpression()), !dbg !40
+
+  %C = and i32 %B, 15, !dbg !41
+  call void @llvm.dbg.value(metadata i32 %C, metadata !37, metadata !DIExpression()), !dbg !41
+
+  ; Preserve the dbg.value for the DCE'd 32-bit 'and'.
+  ;
+  ; The high 16 bits of the 'and' require sign-extending the new 16-bit and:
+  ; CHECK-NEXT: call void @llvm.dbg.value(metadata i16 [[and]], metadata [[C:![0-9]+]],
+  ; CHECK-SAME:    metadata !DIExpression(DW_OP_constu, 15, DW_OP_shr, DW_OP_constu, 4294967295, DW_OP_mul, DW_OP_stack_value, DW_OP_LLVM_fragment, 16, 16))
+  ;
+  ; The low 16 bits of the 'and' are exactly equal to the result of the 16-bit 'and':
+  ; CHECK-NEXT: call void @llvm.dbg.value(metadata i16 [[and]], metadata [[C]], metadata !DIExpression(DW_OP_LLVM_fragment, 0, 16))
+
+  %D = trunc i32 %C to i16, !dbg !42
+  call void @llvm.dbg.value(metadata i16 %D, metadata !38, metadata !DIExpression()), !dbg !42
+
+  ; The dbg.value for a truncate should simply point to the result of the 16-bit 'and'.
+  ; CHECK-NEXT: call void @llvm.dbg.value(metadata i16 [[and]], metadata [[D:![0-9]+]], metadata !DIExpression())
+
+  ret i16 %D, !dbg !43
+  ; CHECK-NEXT: ret i16 [[and]]
+}
+
+declare void @llvm.dbg.value(metadata, metadata, metadata)
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!5}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C, file: !1, producer: "debugify", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2)
+!1 = !DIFile(filename: "void", directory: "/")
+!2 = !{}
+!5 = !{i32 2, !"Debug Info Version", i32 3}
+!7 = !DISubroutineType(types: !2)
+!10 = !DIBasicType(name: "ty32", size: 32, encoding: DW_ATE_signed)
+!12 = !DIBasicType(name: "ty8", size: 8, encoding: DW_ATE_signed)
+!34 = distinct !DISubprogram(name: "test5", linkageName: "test5", scope: null, file: !1, line: 12, type: !7, isLocal: false, isDefinition: true, scopeLine: 12, isOptimized: true, unit: !0, retainedNodes: !35)
+!35 = !{!36, !37, !38}
+!36 = !DILocalVariable(name: "B", scope: !34, file: !1, line: 12, type: !10)
+!37 = !DILocalVariable(name: "C", scope: !34, file: !1, line: 13, type: !10)
+!38 = !DILocalVariable(name: "D", scope: !34, file: !1, line: 14, type: !39)
+!39 = !DIBasicType(name: "ty16", size: 16, encoding: DW_ATE_signed)
+!40 = !DILocation(line: 12, column: 1, scope: !34)
+!41 = !DILocation(line: 13, column: 1, scope: !34)
+!42 = !DILocation(line: 14, column: 1, scope: !34)
+!43 = !DILocation(line: 15, column: 1, scope: !34)

test/Transforms/InstCombine/dce-iterate.ll

-; RUN: opt < %s -instcombine -S | grep "ret double .sy"
+; RUN: opt < %s -debugify -instcombine -S | FileCheck %s
 
+; CHECK-LABEL: define {{.*}} @ScaleObjectAdd
 define internal double @ScaleObjectAdd(double %sx, double %sy, double %sz) nounwind {
 entry:
         %sx34 = bitcast double %sx to i64               ; <i64> [#uses=1]
         %sx3435 = zext i64 %sx34 to i960                ; <i960> [#uses=1]
+
+; The i64 -> i960 zero extension is DCE'd. Check that we preserve debug values
+; for the intermediate SSA values.
+;
+; Bits 64..960 of the zext (just 0):
+; CHECK: call void @llvm.dbg.value(metadata i960 0, metadata [[sx3435:![0-9]+]], metadata !DIExpression(DW_OP_LLVM_fragment, 64, 896))
+;
+; Bits 1..64 of the zext. This is the bitcasted double %sx.
+; CHECK-NEXT: call void @llvm.dbg.value(metadata double %sx, metadata [[sx3435]], metadata !DIExpression(DW_OP_LLVM_fragment, 0, 64))
+
         %sy22 = bitcast double %sy to i64               ; <i64> [#uses=1]
         %sy2223 = zext i64 %sy22 to i960                ; <i960> [#uses=1]
         %sy222324 = shl i960 %sy2223, 320               ; <i960> [#uses=1]
         %sy222324.ins = or i960 %sx3435, %sy222324              ; <i960> [#uses=1]
         %sz10 = bitcast double %sz to i64               ; <i64> [#uses=1]
         %sz1011 = zext i64 %sz10 to i960                ; <i960> [#uses=1]
         %sz101112 = shl i960 %sz1011, 640               ; <i960> [#uses=1]
         %sz101112.ins = or i960 %sy222324.ins, %sz101112 
         
         %a = trunc i960 %sz101112.ins to i64            ; <i64> [#uses=1]
         %b = bitcast i64 %a to double           ; <double> [#uses=1]
         %c = lshr i960 %sz101112.ins, 320               ; <i960> [#uses=1]
         %d = trunc i960 %c to i64               ; <i64> [#uses=1]
         %e = bitcast i64 %d to double           ; <double> [#uses=1]
         %f = fadd double %b, %e
 
+; CHECK: ret double %sy
         ret double %e
 }
+
+; CHECK: [[sx3435]] = !DILocalVariable(name: "2"