[PATCH] D91734: [FastISel] Flush local value map on every instruction

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

In D91734#2426897 <https://reviews.llvm.org/D91734#2426897>, @rnk wrote:

> I see. We should give that constant materialization a location.

Yeah, likely - if this patch makes constant materialization local to the IR instruction - if there's a reasonable location the IR instruction should have, that seems fair to me.

(a more general fix (that would cover cases where the instruction really has no location) might be to propagate locations from the first instruction in a basic block with a location back up to the start of the basic block - I forget if we've considered/tried that before)

> It looks like it is coming from a phi node. The IR looks like this:
>
>     %6 = icmp ne i32 %5, 0, !dbg !11
>     br i1 %6, label %7, label %10, !dbg !12
>   
>   7:                                                ; preds = %2
>     %8 = load i32, i32* %4, align 4, !dbg !13
>     %9 = icmp ne i32 %8, 0, !dbg !13
>     br label %10
>   
>   10:                                               ; preds = %7, %2
>     %11 = phi i1 [ false, %2 ], [ %9, %7 ], !dbg !14   ;;;; Probably where the zero comes from
>     %12 = zext i1 %11 to i32, !dbg !11
>     ret i32 %12, !dbg !15
>
> The PHI node has location !14, which is a line 0 location. Is there a reason we give this PHI a line 0 location, when it's built by the frontend for the conditional operator? IMO we should use the location of the `br` instruction, which will be the location of the conditional operator (`&&` at the source level).
>
> Paul has already shown that flushing the local value map improves debug info quality in general. If we can't fix all the gdb test suite failures, IMO we should consider XFAILing them and moving on.

For sure there's tipping points where one bug is worth another - but generally it's best to avoid that if possible. (& a sliding scale of "can be fixed" - matter of code, complexity, etc - yeah, if the alternative is rewriting big parts of LLVM to avoid regressing one thing to progress here - I'm with you, there's some place where we'd tradeoff some bugs for some other bugs)

In D91734#2429144 <https://reviews.llvm.org/D91734#2429144>, @probinson wrote:

> Just for grins, change the `&&` to `||` and see what happens...
> The xorl becomes a movb $1 (no surprise there).  But, that instruction no longer has line-0, instead it becomes part of the prologue.
> This tells me that the xorl had an explicit line 0, while the 'movb $1' has no location (a subtle but important difference).
>
> There are also curious differences between the CGExprScalar.cpp methods VisitBinLAnd() and VisitBinLOr(); the former is more attentive to line attributions than the latter, which seems likely to be an oversight dating back a decade or so.
>
> Onward into the breach.

I did some work years back to try to streamline the location of instructions when code generating expressions in clang. Mostly centered around the creation and use of `ApplyDebugLocation` scoped type. Which you can see used in various places, but for instance, at the start of `CodeGenFunction::EmitLValue(const Expr *E)` - the idea being that all instructions related to code generating that expression would have a location the same as the preferred location (the same place clang points to in error messages) of the expression - and any subexpression would get a similar treatment, overriding the outer expression's location in turn.

Let's see what's up with VisitBinLAnd and VisitBinLOr... Ah, you're referring to the use of ApplyDebugLocation around the two EmitBlock calls in VisitBinLAnd that are missing in VisitBinLOr? Yeah, looks like an omission on my part back when - I vaguely remember that whole "unconditional branches shouldn't have locations" thing, but hopefully there's enough info in the commit messages to help explain why it's useful as I don't think I have that much context anymore.

Right you are about line 0 V no location - the phi after the && has line 0, but the phi after the || has no line.

Here's where the line 0 for the Phi is coming from: https://github.com/llvm-mirror/clang/blob/master/lib/CodeGen/CGExprScalar.cpp#L4039 - I was going to blame myself, but seems to have come from here: https://reviews.llvm.org/D47720

This patch might be undoing, awkwardly, the point of my original patch of omitting locations on unconditional jumps... Hmm, nope, that's just about the jumps.

Looks like this Phi creation ( https://github.com/llvm-mirror/clang/blob/master/lib/CodeGen/CGExprScalar.cpp#L4012 ) misses the opportunity to get the location from the expression, because it's not created by the IRBuilder - perhaps it should be, rather than being given an artificial location - we know which expression it's coming from, just as much as we know the instruction for the loads, etc. So maybe that code should be `ApplyDebugLocation::CreateArtificial` and scope, and instead set the location to the current debug location - probably move it up to where the Phi is being constructed too - I wouldn't've thought there was a need to move it away from that point.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91734/new/

https://reviews.llvm.org/D91734