Fix a LSR debug invariance issue

[markus]

Summary

In response to https://github.com/llvm/llvm-project/issues/56030

To be a bit more precise: LSR debug salvaging (DbgGatherSalvagableDVI) will call ScalarEvolution::getSCEV on the operands of @llvm.dbg.value. When doing this the results (SCEV -> Value and Value -> SCEV) are cached by the SCEV framework.

Later on when generating IR for the transformed loop LSRInstance::Expand ends up in SCEVExpander::FindValueInExprValueMap where the cached %inc = add i32 %0, 1 is found for SCEV (1 + %0) and that IR gets reused.

For the case where there is no @llvm.dbg.value in the input then %inc is not queried, not cached and wont be found when expanding. This results in a debug invariance issue.

If this is indeed the problem then I see two possible solutions. Either having SCEV forget about the query (and remove it from the cache) or some interface to prevent SCEV caching it in the first place.

[jmorse]

Thanks for hunting this down, caching layers affecting codegen sounds hard to debug.

I'm no SCEV expert, so a couple of possibly obvious questions:

• Is it safe to store the SCEV* for a Value that's then forgotten?
• Is there a risk that the presence of debug-info will now cause a SCEV to be forgotten that _should_ have been remembered, because of unconditionally forgetting a SCEV if it's used by debug-info?

[markus]

In D129636#3648003, @jmorse wrote:

Thanks for hunting this down, caching layers affecting codegen sounds hard to debug.

I'm no SCEV expert, so a couple of possibly obvious questions:

• Is it safe to store the SCEV* for a Value that's then forgotten?
• Is there a risk that the presence of debug-info will now cause a SCEV to be forgotten that _should_ have been remembered, because of unconditionally forgetting a SCEV if it's used by debug-info?

Unsure and possibly :) I am hoping that the other reviewers will bring some clarity to these questions.

For the first question about storing a SCEV I know there was a lot of discussion about that when I did the original patch some years ago (D87494 and D91711) but since then that was fixed and it has afterwards been completely rewritten so not sure what the status is now.

For the second point maybe ScalarEvolution::getExistingSCEV() could be used to see if this SCEV existed before analyzing the dbg.value or not so that we don't end up forgetting about values that we would not forget about hadn't it been for the dbg.value.

[eopXD]

llvm/test/Transforms/LoopStrengthReduce/dbg-inv-0.ll

8

Nit: extra space between were observed.

[markus]

Fix nit.

[eopXD]

May you explain a bit how the test case shows SE.forgetValue() is preserving debug invariance. Thank you.

[markus]

In D129636#3654426, @eopXD wrote:

May you explain a bit how the test case shows SE.forgetValue() is preserving debug invariance. Thank you.

Right. The test case intends to show that for the given input LSR will give the same output regardless if debug info is stripped before running the LSR pass or after running the LSR pass (i.e .the pass being debug invariant).

Before the addition of SE.forgetValue() this was not the case and the test failed.

As I understand it LSR uses SCEVExpander to generate code for some of the expressions used by the new loop and the problem lies in that that SCEVExpander will search among (and reuse) values that have already been analyzed by SCEV (i.e. SE.getSCEV(). Now having debug intrinsics in the input means that we may do SE.getSCEV() on values that we would not have done SE.getSCEV() on otherwise and this may result in SCEVExpander generating different code which is bad.

It is still up for debate if SE.forgetValue() is the right solution but the idea was that if we immediately forget about those values that were analyzed just because of the debug intrinsic they will not end up affecting the SCEVExpander.

[eopXD]

I see, the approach and test case make sense to me now. Thank you for the explanation.

[eopXD]

Probably can have @nikic to give a look before landing this?

[markus]

I'd still like to get some feedback from the author of the LSR debug salvaging (@chrisjackson) before proceeding as at the moment it is a bit unclear if it actually is safe to do a SE.forgetValue() here.

[markus]

ping

[chrisjackson]

In D129636#3654504, @markus wrote:

I'd still like to get some feedback from the author of the LSR debug salvaging (@chrisjackson) before proceeding as at the moment it is a bit unclear if it actually is safe to do a SE.forgetValue() here.

Hello, apologies, this only just came to my attention so I'm looking at it now! Possibly my phab email is misconfigured - I'll check that out and fix it.

Thanks for working on this. This seems a very interesting case. I had naively assumed getSCEV() was benign and would not result in creating extra information for the compiler's code generation. Obviously it is a problem that it does. On first glance your solution looks good, but I'll try and review in more detail.

[chrisjackson]

I'm looking at the forgetValue() implementation. I think that as long as the SCEV for the value is preserved and accessable in SalvageDVI() then all is well in terms of the salvaging method.

In D129636#3648003, @jmorse wrote:

Thanks for hunting this down, caching layers affecting codegen sounds hard to debug.

I'm no SCEV expert, so a couple of possibly obvious questions:

• Is it safe to store the SCEV* for a Value that's then forgotten?
• Is there a risk that the presence of debug-info will now cause a SCEV to be forgotten that _should_ have been remembered, because of unconditionally forgetting a SCEV if it's used by debug-info?

I think the second point is the crux here. Perhaps there is a way to test if the SCEV already exists, then call forget conditionally.

[markus]

In D129636#3696240, @chrisjackson wrote:

I'm looking at the forgetValue() implementation. I think that as long as the SCEV for the value is preserved and accessable in SalvageDVI() then all is well in terms of the salvaging method.

Right, I think that forgetting a SCEV does not invalidate the SCEV itself it is just that if we were to ask for it again then we would get a new object (that would still be equivalent though not the same object pointer wise).

In D129636#3648003, @jmorse wrote:

Thanks for hunting this down, caching layers affecting codegen sounds hard to debug.

I'm no SCEV expert, so a couple of possibly obvious questions:

• Is it safe to store the SCEV* for a Value that's then forgotten?
• Is there a risk that the presence of debug-info will now cause a SCEV to be forgotten that _should_ have been remembered, because of unconditionally forgetting a SCEV if it's used by debug-info?

I think the second point is the crux here. Perhaps there is a way to test if the SCEV already exists, then call forget conditionally.

A few comments up I wrote which I think does what you are requesting

For the second point maybe ScalarEvolution::getExistingSCEV() could be used to see if this SCEV existed before analyzing the dbg.value or not so that we don't end up forgetting about values that we would not forget about hadn't it been for the dbg.value.

If that makes sense I will update the patch.

[chrisjackson]

In D129636#3696260, @markus wrote:

In D129636#3696240, @chrisjackson wrote:

I'm looking at the forgetValue() implementation. I think that as long as the SCEV for the value is preserved and accessable in SalvageDVI() then all is well in terms of the salvaging method.

Right, I think that forgetting a SCEV does not invalidate the SCEV itself it is just that if we were to ask for it again then we would get a new object (that would still be equivalent though not the same object pointer wise).

In D129636#3648003, @jmorse wrote:

Thanks for hunting this down, caching layers affecting codegen sounds hard to debug.

I'm no SCEV expert, so a couple of possibly obvious questions:

• Is it safe to store the SCEV* for a Value that's then forgotten?
• Is there a risk that the presence of debug-info will now cause a SCEV to be forgotten that _should_ have been remembered, because of unconditionally forgetting a SCEV if it's used by debug-info?

I think the second point is the crux here. Perhaps there is a way to test if the SCEV already exists, then call forget conditionally.

A few comments up I wrote which I think does what you are requesting

For the second point maybe ScalarEvolution::getExistingSCEV() could be used to see if this SCEV existed before analyzing the dbg.value or not so that we don't end up forgetting about values that we would not forget about hadn't it been for the dbg.value.

If that makes sense I will update the patch.

Apologies, I overlooked your earlier response to that point. Your solution sounds good to me.

[markus]

Not sure if this ended up so well as it appears that getExistingSCEV was a private method and I am uncertain if we can or should make it public. Either way if we decide to go this route then we probably should add explicit tests for this part as well (i.e. to make sure that the forgetting of SCEV does not produce another invariance issue).

[chrisjackson]

In D129636#3696435, @markus wrote:

Not sure if this ended up so well as it appears that getExistingSCEV was a private method and I am uncertain if we can or should make it public. Either way if we decide to go this route then we probably should add explicit tests for this part as well (i.e. to make sure that the forgetting of SCEV does not produce another invariance issue).

I similarly added a public method in rG116dc70 in an early SCEV-based salvaging patch. Understandably, I don't think it was envisaged at the time of implementation that SCEVs would be used for salvaging. I agree we should be careful.

[fhahn]

I think I am missing some context here, but this looks a bit suspicious. I am not sure if exposing and checking whether a SCEV exists is the right way to go.

IIUC the main issue seems to be that we try to create SCEVs for all dbg intrinsics? Would it be possible to instead only get SCEV expressions for values we really need (that/s the induction variable I assume, for which there already should be SCEV expression)?

[markus]

In D129636#3696750, @fhahn wrote:

I think I am missing some context here, but this looks a bit suspicious. I am not sure if exposing and checking whether a SCEV exists is the right way to go.

Right. I am not so sure this is a good idea either.

The underlaying problem, as I understand it and see it, is that due to the caching in SE.ExprValueMap past calls to getSCEV will affect IR generated by future calls to SCEVExpander and this is not obvious. While it is a reasonable optimization to try and reuse IR during SCEVExpander based on what has already been analyzed I think this 'side-effect' of getSCEV is somewhat unfortunate in the context of debug intrinsics that are not supposed to affect code generation.

Another, also ugly, solution could be to add a flag to getSCEV so that caching can be inhibited for certain calls.

IIUC the main issue seems to be that we try to create SCEVs for all dbg intrinsics? Would it be possible to instead only get SCEV expressions for values we really need (that/s the induction variable I assume, for which there already should be SCEV expression)?

That is possibly a better solution but I do not have sufficient insight into the salvaging algorithm to comment on that (@chrisjackson)

[chrisjackson]

In D129636#3696750, @fhahn wrote:

I think I am missing some context here, but this looks a bit suspicious. I am not sure if exposing and checking whether a SCEV exists is the right way to go.

IIUC the main issue seems to be that we try to create SCEVs for all dbg intrinsics? Would it be possible to instead only get SCEV expressions for values we really need (that/s the induction variable I assume, for which there already should be SCEV expression)?

It is necessary to obtain SCEVs for all the values used in dbg.value within the loop, as LSR may optimise out these values and these are salvaged by generating a DWARF expression from the SCEV expression. For further explanation see:
D105207 and the EuroLLVM2022 talk on the method here :SCEV-Based debuginfo salvaging in Loop Strength Reduction.

[fhahn]

In D129636#3698967, @chrisjackson wrote:

In D129636#3696750, @fhahn wrote:

I think I am missing some context here, but this looks a bit suspicious. I am not sure if exposing and checking whether a SCEV exists is the right way to go.

IIUC the main issue seems to be that we try to create SCEVs for all dbg intrinsics? Would it be possible to instead only get SCEV expressions for values we really need (that/s the induction variable I assume, for which there already should be SCEV expression)?

It is necessary to obtain SCEVs for all the values used in dbg.value within the loop, as LSR may optimise out these values and these are salvaged by generating a DWARF expression from the SCEV expression. For further explanation see:
D105207 and the EuroLLVM2022 talk on the method here :SCEV-Based debuginfo salvaging in Loop Strength Reduction.

Right, but wouldn't it be sufficient to collect all expressions LSR optimizes? For those, it will already have created SCEV expressions for. This might be more difficult, but I am wondering if it would be feasible at least in theory.

[mkazantsev]

The connection between described problem (as far as I understood, the fact that debuginfo changes IR is treated as a problem) and the proposed fix is not clear to me. The patch absolutely needs a detailed explanation, why existing SCEVs is a problem, in the commit message.

Note that, in presence of potential poison values, SCEV as well as many transform passes should treat it correctly. Sometimes "correctly" means "very conservatively", and this is what most likely happened here. Look at this:

https://godbolt.org/z/sbshjW1nM
https://godbolt.org/z/3fM8Exq6x

I've annotated the function's argument as noundef, and now there is no difference b/w presenting and not-presenting dbgs is in no-wrap flags in IR. This flag is there because poison argument of a call is UB by specification (didn't really check for dbg.value, but should be).

The issue will be gone if you annotate incoming parameters as noundef. Does your frontent allow this for this function?

[chrisjackson]

In D129636#3699269, @fhahn wrote:

In D129636#3698967, @chrisjackson wrote:

In D129636#3696750, @fhahn wrote:

I think I am missing some context here, but this looks a bit suspicious. I am not sure if exposing and checking whether a SCEV exists is the right way to go.

IIUC the main issue seems to be that we try to create SCEVs for all dbg intrinsics? Would it be possible to instead only get SCEV expressions for values we really need (that/s the induction variable I assume, for which there already should be SCEV expression)?

It is necessary to obtain SCEVs for all the values used in dbg.value within the loop, as LSR may optimise out these values and these are salvaged by generating a DWARF expression from the SCEV expression. For further explanation see:
D105207 and the EuroLLVM2022 talk on the method here :SCEV-Based debuginfo salvaging in Loop Strength Reduction.

Right, but wouldn't it be sufficient to collect all expressions LSR optimizes? For those, it will already have created SCEV expressions for. This might be more difficult, but I am wondering if it would be feasible at least in theory.

Currently the information for the dbg.value within the loop is cached before LSR executes, with DbgGatherSalvagableDVI(). If the implementation was changed to cache information on only the values that LSR optimises out, then the cacheing code would have to be moved inside the core LSR pass, I believe. I think this would require a significant change in the scev-salvaging implementation and also modifying the LSRInstance interface and implementation. I'm not keen on that idea, but I can have a further think and see if there is a better way.

[markus]

Removed nsw flags from test to avoid confusion. AFAICT they have no relevance here.

[markus]

In D129636#3701940, @mkazantsev wrote:

The connection between described problem (as far as I understood, the fact that debuginfo changes IR is treated as a problem) and the proposed fix is not clear to me. The patch absolutely needs a detailed explanation, why existing SCEVs is a problem, in the commit message.

I have update the review summary.

Note that, in presence of potential poison values, SCEV as well as many transform passes should treat it correctly. Sometimes "correctly" means "very conservatively", and this is what most likely happened here. Look at this:

https://godbolt.org/z/sbshjW1nM
https://godbolt.org/z/3fM8Exq6x

I've annotated the function's argument as noundef, and now there is no difference b/w presenting and not-presenting dbgs is in no-wrap flags in IR. This flag is there because poison argument of a call is UB by specification (didn't really check for dbg.value, but should be).

The issue will be gone if you annotate incoming parameters as noundef. Does your frontent allow this for this function?

I'm afraid that I do not understand this part at all. AFAICT the no-wrap flags are unrelated.

[chrisjackson]

I think this is a succinct solution and I don't perceive a problem with exposing the extra SCEV method, as I've done something similar myself in the past, as mentioned above. I would like to see a comment explaining the need to call forget(), otherwise lgtm.

[markus]

Added code comments.

[mkazantsev]

Did you consider that forgetValue in fact does not only forget the value you are aiming, but also all its users recursively? Not SCEV users (they likely don't exist), but instruction users.

Imagine that some of the users had no-wrap flags inferred from other queries. Now you forget it, and the flags get lost. Won't this introruce a similar issue in another place? It may also affect some loops into which these values are involved.

Generally, exposing API about existing SCEVs isn't a great idea, and I don't see enough justification in your case.

The best alternative I can suggest is to create a new ScalarEvolution instance and use it for dbg value queries. It should be good enough if the only purpose of that is to find undefs.

Anyways, why do you need SCEV at all to find undefs? Can we instead traverse through instructions and get rid of SCEV query at all?

[markus]

In D129636#3715417, @mkazantsev wrote:

The best alternative I can suggest is to create a new ScalarEvolution instance and use it for dbg value queries. It should be good enough if the only purpose of that is to find undefs.

That does sound like a very tempting alternative to be sure that these sort of issues do not crop up again. Not sure if it is possible though (@chrisjackson?)

[chrisjackson]

In D129636#3715417, @mkazantsev wrote:

Did you consider that forgetValue in fact does not only forget the value you are aiming, but also all its users recursively? Not SCEV users (they likely don't exist), but instruction users.

Imagine that some of the users had no-wrap flags inferred from other queries. Now you forget it, and the flags get lost. Won't this introruce a similar issue in another place? It may also affect some loops into which these values are involved.

Generally, exposing API about existing SCEVs isn't a great idea, and I don't see enough justification in your case.

The best alternative I can suggest is to create a new ScalarEvolution instance and use it for dbg value queries. It should be good enough if the only purpose of that is to find undefs.

Anyways, why do you need SCEV at all to find undefs? Can we instead traverse through instructions and get rid of SCEV query at all?

SCEV is not being used to find undefs. The SCEV for a location referenced in a dbg.value is recorded, and if that location is optimised away, then the SCEV is used to generate a DWARF expression that computes the value of the locations and allows the dbg.value to be retained instead of dropped.

[chrisjackson]

In D129636#3715477, @markus wrote:

In D129636#3715417, @mkazantsev wrote:

The best alternative I can suggest is to create a new ScalarEvolution instance and use it for dbg value queries. It should be good enough if the only purpose of that is to find undefs.

That does sound like a very tempting alternative to be sure that these sort of issues do not crop up again. Not sure if it is possible though (@chrisjackson?)

I don't like the sound of duplicaring an entire ScalarEvolution, at first glance it seems expensive to me. I prefer a solution that avoids side-effects of obtaining the SCEV for a value.

[StephenTozer]

I think from the discussion so far, I would personally think the best answer would be to take the alternative route of modifying getSCEV to be able to not add items to the map when a flag IsDebug or such is added. It may be slightly difficult since createSCEVIter modifies the map as it recursively traverses the Value's operands and uses the modifications to complete the requested SCEV. That problem is even worse for this solution however; suppose instead of %inc, the debug value referred to a value %m = mul i32 %inc 2; in that case, getScev(%m) would cache a SCEV expr for %inc along the way, and we wouldn't be calling forgetValue on it since it's not the value we requested! I'd say it's definitely better to have all the messiness confined to just getSCEV and make a slight interface modification personally. Though I'm also surprised that just adding something to the cache is modifying the output - I would have expected that the newly cached SCEV expr would be used only if necessary (i.e. we would otherwise need to generate instructions), and if it was necessary then SCEV would search for an expression regardless. Is this a bug in SCEV, or just a gap left for performance reasons?

[jmorse]

Did you consider that forgetValue in fact does not only forget the value you are aiming, but also all its users recursively? Not SCEV users (they likely don't exist), but instruction users.

Ah, that rather sounds like a blocker -- if there's no way to limit what's forgotten to "just the things unique to this Value", it's going to be impossible to peel apart "what was important to debug-info" and "what was important to codegen". A quick look at the SCEV methods suggests that plumbing in an argument flag is going to be pretty invasive, ScalarEvolution::getSCEV fans out to a lot of functions with a lot of logic.

Stephen wrote:

Though I'm also surprised that just adding something to the cache is modifying the output

I imagine you might find a different way expanding expressions for Values, depending on which PHIs you look at first, which changes if you look at the dbg.values first.

The best alternative I can suggest is to create a new ScalarEvolution instance and use it for dbg value queries. It should be good enough if the only purpose of that is to find undefs.

I'm in two minds about this -- it solves all the problems raised (obviously), on the other hand I imagine it doubles the amount of work done, which isn't ideal. However, I think the reason why we're using ScalarEvolution in this way is because all the PHIs might get replaced during loop-strength-reduction, and we need to use ScalarEvolution's map of old-to-new PHIs to reconstruct the dbg.values operands -- is that right @chrisjackson ?

[StephenTozer]

In D129636#3716487, @jmorse wrote:

I imagine you might find a different way expanding expressions for Values, depending on which PHIs you look at first, which changes if you look at the dbg.values first.

I thought that at first and was wondering if it was just an order-determined thing, which is always possible but could potentially be avoided by using something other than insertion order; in this case though it looks as though in the absence of the debug value, it does choose to insert a new instruction (%4 = add i32 %0, 1) when a valid one already exists (%inc = add nsw i32 %0, 1). It's not particularly relevant, since the existing behaviour is what it is and the patch needs to take that into account, it's just a slight surprise-factor that calling a get function and/or adding an item to a cache causes an instruction to be removed, although in the end it's almost sure to be removed anyway.

[chrisjackson]

In D129636#3716487, @jmorse wrote:

Did you consider that forgetValue in fact does not only forget the value you are aiming, but also all its users recursively? Not SCEV users (they likely don't exist), but instruction users.

Ah, that rather sounds like a blocker -- if there's no way to limit what's forgotten to "just the things unique to this Value", it's going to be impossible to peel apart "what was important to debug-info" and "what was important to codegen". A quick look at the SCEV methods suggests that plumbing in an argument flag is going to be pretty invasive, ScalarEvolution::getSCEV fans out to a lot of functions with a lot of logic.

Stephen wrote:

Though I'm also surprised that just adding something to the cache is modifying the output

I imagine you might find a different way expanding expressions for Values, depending on which PHIs you look at first, which changes if you look at the dbg.values first.

The best alternative I can suggest is to create a new ScalarEvolution instance and use it for dbg value queries. It should be good enough if the only purpose of that is to find undefs.

I'm in two minds about this -- it solves all the problems raised (obviously), on the other hand I imagine it doubles the amount of work done, which isn't ideal. However, I think the reason why we're using ScalarEvolution in this way is because all the PHIs might get replaced during loop-strength-reduction, and we need to use ScalarEvolution's map of old-to-new PHIs to reconstruct the dbg.values operands -- is that right @chrisjackson ?

That's correct. LSR can rewrite PHIs and create new Induction Variables (IVs), eliminating the old ones. Combining the LSR IVs with the SCEVs obtained for optimised-out locations enables reconstruction of the values.

llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp

for (auto &I : *B) {
       // Each location Op may need a SCEVDbgValueBuilder in order to recover it.
       // Pre-allocating a vector will enable quick lookups of the builder later
       // during the salvage.
       NewRec->RecoveryExprs.resize(DVI->getNumVariableLocationOps());
       for (const auto LocOp : DVI->location_ops()) {
         NewRec->SCEVs.push_back(SE.getSCEV(LocOp));
         NewRec->LocationOps.push_back(LocOp);
         NewRec->HadLocationArgList = DVI->hasArgList();
+        SE.forgetValue(LocOp);
       }
       SalvageableDVISCEVs.push_back(std::move(NewRec));
       DVIHandles.insert(DVI);
     }
   }
 }
 
 /// Ideally pick the PHI IV inserted by ScalarEvolutionExpander. As a fallback

llvm/test/Transforms/LoopStrengthReduce/dbg-inv-0.ll

+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt < %s -loop-reduce -S | opt -S -strip-debug | FileCheck %s
+; RUN: opt < %s -strip-debug -S | opt -S -loop-reduce | FileCheck %s
+
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+
+; Verify that transformation is debug invariant. More specifically problems
+; were  observed where SCEV had cached expressions during debug salvage analysis
+; (which only happens in presence of debug intrinsics) and then during
+; SCEVExpand these cached results were found and reused instead of new
+; instructions being generated for the transformed loop.
+
+define void @k(ptr %f) {
+; CHECK-LABEL: @k(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[TMP0:%.*]] = load i32, ptr [[F:%.*]], align 1
+; CHECK-NEXT:    [[INC:%.*]] = add nsw i32 [[TMP0]], 1
+; CHECK-NEXT:    store i32 [[INC]], ptr [[F]], align 1
+; CHECK-NEXT:    br label [[FOR_COND_CLEANUP:%.*]]
+; CHECK:       for.cond.cleanup:
+; CHECK-NEXT:    [[TMP1:%.*]] = add i32 [[TMP0]], 1
+; CHECK-NEXT:    br label [[FOR_BODY3:%.*]]
+; CHECK:       for.body3:
+; CHECK-NEXT:    [[LSR_IV:%.*]] = phi i32 [ [[LSR_IV_NEXT:%.*]], [[FOR_BODY3]] ], [ [[TMP1]], [[FOR_COND_CLEANUP]] ]
+; CHECK-NEXT:    store i32 [[LSR_IV]], ptr null, align 1
+; CHECK-NEXT:    [[LSR_IV_NEXT]] = add i32 [[LSR_IV]], 1
+; CHECK-NEXT:    br label [[FOR_BODY3]]
+;
+entry:
+  %0 = load i32, ptr %f, align 1
+  %inc = add nsw i32 %0, 1
+  store i32 %inc, ptr %f, align 1
+  br label %for.cond.cleanup
+
+for.cond.cleanup:                                 ; preds = %entry
+  br label %for.body3
+
+for.body3:                                        ; preds = %for.body3, %for.cond.cleanup
+  %1 = phi i32 [ %0, %for.cond.cleanup ], [ %inc4, %for.body3 ]
+  %inc4 = add nsw i32 %1, 1
+  store i32 %inc4, ptr null, align 1
+  call void @llvm.dbg.value(metadata i32 %inc, metadata !7, metadata !DIExpression(DW_OP_LLVM_convert, 32, DW_ATE_unsigned, DW_OP_LLVM_convert, 16, DW_ATE_unsigned, DW_OP_stack_value)), !dbg !18
+  br label %for.body3
+}
+
+; Function Attrs: nocallback nofree nosync nounwind readnone speculatable willreturn
+declare void @llvm.dbg.value(metadata, metadata, metadata) #0
+
+attributes #0 = { nocallback nofree nosync nounwind readnone speculatable willreturn }
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4, !5, !6}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, globals: !2, splitDebugInlining: false, nameTableKind: None)
+!1 = !DIFile(filename: "foo.c", directory: "/tmp")
+!2 = !{}
+!3 = !{i32 7, !"Dwarf Version", i32 4}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !{i32 1, !"wchar_size", i32 1}
+!6 = !{i32 7, !"frame-pointer", i32 2}
+!7 = !DILocalVariable(name: "m", scope: !8, file: !1, line: 12, type: !15)
+!8 = distinct !DISubprogram(name: "k", scope: !1, file: !1, line: 7, type: !9, scopeLine: 7, flags: DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !11)
+!9 = !DISubroutineType(types: !10)
+!10 = !{null}
+!11 = !{!12, !7, !16}
+!12 = !DILocalVariable(name: "l", scope: !13, file: !1, line: 10, type: !14)
+!13 = distinct !DILexicalBlock(scope: !8, file: !1, line: 10, column: 3)
+!14 = !DIDerivedType(tag: DW_TAG_volatile_type, baseType: !15)
+!15 = !DIBasicType(name: "int", size: 16, encoding: DW_ATE_signed)
+!16 = !DILocalVariable(name: "n", scope: !17, file: !1, line: 13, type: !15)
+!17 = distinct !DILexicalBlock(scope: !8, file: !1, line: 13, column: 3)
+!18 = !DILocation(line: 0, scope: !8)