Page MenuHomePhabricator
Differential D78369 Diff 265778 llvm/include/llvm/Transforms/Utils/Local.h

Changeset View

Standalone View

View Options

llvm/include/llvm/Transforms/Utils/Local.h

Show First 20 Lines • Show All 91 Lines • ▼ Show 20 Lines
void replaceDbgValueForAlloca(AllocaInst *AI, Value *NewAllocaAddress, void replaceDbgValueForAlloca(AllocaInst *AI, Value *NewAllocaAddress,
DIBuilder &Builder, int Offset = 0); DIBuilder &Builder, int Offset = 0);
/// Finds alloca where the value comes from. /// Finds alloca where the value comes from.
AllocaInst *findAllocaForValue(Value *V, AllocaInst *findAllocaForValue(Value *V,
DenseMap<Value *, AllocaInst *> &AllocaForValue); DenseMap<Value *, AllocaInst *> &AllocaForValue);
/// Assuming the instruction \p I is going to be deleted, attempt to salvage /// Assuming the instruction \p I is going to be deleted, attempt to salvage
/// debug users of \p I by writing the effect of \p I in a DIExpression. /// debug users of \p I by writing the effect of \p I in a DIExpression. If it
/// Returns true if any debug users were updated. /// cannot be salvaged mark it undef.
vskUnsubmitted
Not Done
ReplyInline Actions

Maybe 's/mark it/change its debug uses to/'?

vsk: Maybe 's/mark it/change its debug uses to/'?
bool salvageDebugInfo(Instruction &I); void salvageDebugInfo(Instruction &I);
/// Salvage all debug users of the instruction \p I or mark it as undef if it
/// cannot be salvaged.
void salvageDebugInfoOrMarkUndef(Instruction &I);
aprantlUnsubmitted
Not Done
ReplyInline Actions

Should we rename this to updateDebugInfoForDeletedInsn() or some variant of that to make it more clear that it isn't optional to call this method?

aprantl: Should we rename this to `updateDebugInfoForDeletedInsn()` or some variant of that to make it…
vskUnsubmitted
Not Done
ReplyInline Actions

I don't think the current name is unclear (I'm not opposed to changing it, just don't see a strong need to right now).

However, what /does/ seem missing is some way to enforce that this method is called. I like @alok's approach in D70419: wdyt of just making Instruction::eraseFromParent() call Instruction::salvageDebugInfo()? (This would be for a follow-up, my preference would be to consolidate APIs here first.)

vsk: I don't think the current name is unclear (I'm not opposed to changing it, just don't see a…
vskUnsubmitted
Done
ReplyInline Actions

Ah, one issue with this is that the common deletion idiom is: "I.RAUW(replacement); I.eraseFromParent();" -- so if the salageDebugInfo operation occurs in eraseFromParent, in many cases it's too late, because the debug uses are gone.

vsk: Ah, one issue with this is that the common deletion idiom is: "I.RAUW(replacement); I.
jmorseUnsubmitted
Done
ReplyInline Actions

I've no opinion on changing the name; for the "RAUW; Erase" deletion idiom I don't think that's a problem, it just means those locations were saved by the RAUW instead of through salvaging. A salvage call in eraseFromParent would be highly useful in catching the more complicated circumstances, for example I believe that during inlining large numbers of instructions are pruned for being unused, without ever being RAUW'd.

jmorse: I've no opinion on changing the name; for the "RAUW; Erase" deletion idiom I don't think that's…
chrisjacksonAuthorUnsubmitted
Done
ReplyInline Actions

Yes this is something I'd like to address too.

chrisjackson: Yes this is something I'd like to address too.
chrisjacksonAuthorUnsubmitted
Done
ReplyInline Actions

I'm not sure that a method name is a good way of reflecting its necessity.

chrisjackson: I'm not sure that a method name is a good way of reflecting its necessity.
dblaikieUnsubmitted
Not Done
ReplyInline Actions

I'll +1 @aprantl's comment - it sounds like at least from the comment the previous version of this function was "best effort" - if you didn't call it you just got worse debug info. But the new behavior seems like it might break (result in incorrect debug info) if you don't call this function? Is that the case? (if so, then a new name seems justified)

dblaikie: I'll +1 @aprantl's comment - it sounds like at least from the comment the previous version of…
OrlandoUnsubmitted
Not Done
ReplyInline Actions

it sounds like at least from the comment the previous version of this
function was "best effort" - if you didn't call it you just got worse debug
info.

This is only true if replaceDbgUsesWithUndef was being called where
appropriate. With D80264 we do actually get this for free in a lot of cases,
but there are still times where we want to undef a dbg.value without deleting
the instruction. E.g. When sinking an instruction with a corresponding
dbg.value, we want to leave behind a salvage-or-undef, and sink a clone
of the original dbg.value to the new instruction position.

Orlando: > it sounds like at least from the comment the previous version of this > function was "best…
dblaikieUnsubmitted
Not Done
ReplyInline Actions

it sounds like at least from the comment the previous version of this

function was "best effort" - if you didn't call it you just got worse debug
info.

This is only true if replaceDbgUsesWithUndef was being called where

appropriate.

Sorry, not sure I understand - you mean previously you had to call one of either replaceDbgUsesWithUndef (not anymore, since D80264, now that's implicit) or salvageDebugInfo - and now you have to always call salvageDebugInfo ?

(maybe I shuold just bow out of this review - I haven't gotten very involved in LLVM's debug info variable location tracking, so I don't have a lot of context for this - just wanted to express some agreement with @aprantl's perspective there)

dblaikie: >> it sounds like at least from the comment the previous version of this function was "best…
OrlandoUnsubmitted
Not Done
ReplyInline Actions

Ah sorry for being unclear, I hope this helps. I don't have a strong opinion on the name change.

AFAIK it is correct to call replaceDbgUsesWithUndef if salvageDebugInfo failed (returns false), and not doing so can lead to wrong debug-info by allowing previous dbg.value definitions to run on for too long. This patch changes salvageDebugInfo to call replaceDbgUsesWithUndef itself if it fails to salvage. Because replaceDbgUsesWithUndef is now called automatically when we delete an instruction (D80264), not calling salvageDebugInfo just reduces coverage, but doesn't produce incorrect debug-info when deleting instructions.

But the new behavior seems like it might break (result in incorrect debug info) if you don't call this function? Is that the case? (if so, then a new name seems justified)

So I think the answer to this is no, now that we have D80264.

Orlando: Ah sorry for being unclear, I hope this helps. I don't have a strong opinion on the name change.
/// Implementation of salvageDebugInfo, applying only to instructions in /// Implementation of salvageDebugInfo, applying only to instructions in
/// \p Insns, rather than all debug users of \p I. /// \p Insns, rather than all debug users of \p I.
bool salvageDebugInfoForDbgValues(Instruction &I, bool salvageDebugInfoForDbgValues(Instruction &I,
ArrayRef<DbgVariableIntrinsic *> Insns); ArrayRef<DbgVariableIntrinsic *> Insns);
/// Given an instruction \p I and DIExpression \p DIExpr operating on it, write /// Given an instruction \p I and DIExpression \p DIExpr operating on it, write
/// the effects of \p I into the returned DIExpression, or return nullptr if /// the effects of \p I into the returned DIExpression, or return nullptr if
/// it cannot be salvaged. \p StackVal: whether DW_OP_stack_value should be /// it cannot be salvaged. \p StackVal: whether DW_OP_stack_value should be
▲ Show 20 Lines • Show All 90 Lines • Show Last 20 Lines