[PATCH] D39345: SCEV: preserve debug information attached to PHI nodes.

[Hal Finkel via Phabricator via llvm-commits]

hfinkel added a comment.

First, should this logic be here or should it be in IndVarSimplify? I ask because there are transformations that can introduce new AddRecs into an existing loop without replacing the induction variable. Only the transformation using the SCEVExpander knows if it is planning to actually replace the induction variable.

A related question: Is it okay to have multiple values in a function define the same dbg.value's variable? If, for example, we were to end up with multiple induction variables in a loop, and all had a relationship to the source-level induction variable, would it be okay if all these PHIs were used by dbg.value intrinsics to define the same source-level variable? Or would that just confuse things?

In your test case, IndVarSimplify is doing induction-variable widening on an already-canonical induction variable. This is an interesting special case because both the old and new induction variables are canonical (i.e., they both start from zero and increment by one on each iteration). One thing that makes it special is that there already is an old canonical induction variable when the new one is added. In general, for IndVarSimplify, we won't have an existing canonical induction variable (we'll just have some other AddRecs - non-canonical induction variables) and we'll be creating the canonical induction variable.

Generally, for IndVarSimplify, you'll have some AddRec corresponding to some existing PHI - the non-canonical induction variable with the debug info - and IndVarSimplify will introduce a canonical induction variable. The AddRec for the existing PHI {a,+,b} is implicitly defined in terms of the canonical induction variable just created: OldPN = a + b*CanPN. So, the dbg.value defining the variable previously defined by OldPN can be defined now by a DWARF expression a + b*CanPN (plus, perhaps any trunctions/extensions).

Also, in your test case here for induction-variable widening, should the DWARF expression after this have some kind of DIExpression with a truncate in it to reverse the effect of the widening?



================
Comment at: lib/Analysis/ScalarEvolutionExpander.cpp:1301
+  // Attach any debug information to the new PHI.
+  if (PhiSCEV) {
+    auto *NewPhiSCEV = dyn_cast<SCEVAddRecExpr>(SE.getSCEV(PN));
----------------
`L->getCanonicalInductionVariable()` can fail (i.e., return nullptr), so I imagine this guard could/should be:

  if (PhiSCEV && OldPN) {

(the goal of IndVarSimplify is to introduce such a variable, but it might not exist, especially before/during IndVarSimplify running)


https://reviews.llvm.org/D39345