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

[Paul Robinson via Phabricator via lldb-commits]

probinson added a comment.

> And with these changes together, breaking on the function breaks on line 5 (presumably because this is creating the constant used by the second '&&' which is on line 5) instead of line 3. Not the worst thing, but I imagine given Sony's interest in less "jumpy" line tables, this might not be super desirable.

It's breaking on an `xorl %eax,%eax` which is produced by the PHI at the end of the conditional expression, which now has the source location of the operator at the top of the expression tree, which is the second `&&` operator, yeah.  Not the best.  (FTR, the jumpiness complaints we get are usually more egregious, hopping between different source statements somewhat arbitrarily; not sure anyone would complain too loudly about hopping around within an expression.  We see less of that in any case, because we suppress column info, but still.)

The real bear here is getting that constant to behave reasonably.  In all cases the sequence of instructions is the same, but silly things keep happening to the line table.  (As an aside, GCC does this entire thing with control-flow, not trying to compute and then test the bool result of the expression.  They also handle `a--` better.  But I digress.)

Current HEAD puts the xor in the prologue (which also puts the while-loop top *before* prologue_end, that can't be right).  Adding the PHI change doesn't affect this, because FastISel is still materializing the zero in the local value map, which (in HEAD) has no source locations.

  .LBB0_1:                                # %while.cond
  	xorl	%eax, %eax
  .Ltmp0:
  	.loc	1 3 10 prologue_end             # multi-line-while.c:3:10
  	cmpl	$0, -4(%rbp)

With just my FastISel change (D91734 <https://reviews.llvm.org/D91734>), prologue_end is better, but the xor has line-0, which makes gdb sad (it decides the entire function has no source locations, which is wrong, but whatever):

  .LBB0_1:                                # %while.cond
  .Ltmp0:
  	.loc	1 0 0 prologue_end              # multi-line-while.c:0:0
  	xorl	%eax, %eax
  	.loc	1 3 10                          # multi-line-while.c:3:10
  	cmpl	$0, -4(%rbp)

With the PHI change from D92606 <https://reviews.llvm.org/D92606>, plus D91734 <https://reviews.llvm.org/D91734>, we see the PHI taking effect, which means we first break on line 5 and then hop backwards:

  .LBB0_1:                                # %while.cond
  .Ltmp0:
  	.loc	1 5 10 prologue_end             # multi-line-while.c:5:10
  	xorl	%eax, %eax
  	.loc	1 3 10                          # multi-line-while.c:3:10
  	cmpl	$0, -4(%rbp)

I think we'd prefer something more like this:

  .LBB0_1:                                # %while.cond
  .Ltmp0:
  	.loc	1 3 10 prologue_end             # multi-line-while.c:3:10
  	xorl	%eax, %eax
  	cmpl	$0, -4(%rbp)

Getting that to happen means not making the PHI change, and persuading FastISel to do something more intelligent.  Flushing on every instruction did seem to help, maybe if we made sure that first instruction also had the source location of the first "real" instruction?  I will experiment with this tomorrow.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91734