[PATCH] [DebugInfo][FastISel] Prevent using debug location from previous block for local values

[Sergey Dmitrouk]

> Some comments


Thanks, they are addressed in the next revision.

> it'd still be good to have Alexey take a look and see if this is likely to address some of the issues in line table fidelity he knows about.


I'll subscribe him, maybe he'll find time to take a look.


REPOSITORY
  rL LLVM

================
Comment at: lib/CodeGen/SelectionDAG/FastISel.cpp:136
@@ +135,3 @@
+    // Skip past any EH_LABELs, which go before local values.
+    while (FirstLocalValue->getOpcode() == TargetOpcode::EH_LABEL)
+      ++FirstLocalValue;
----------------
dblaikie wrote:
> I don't see anything EH related in your test cases - is this exercised/verified? (perhaps without this existing tests regress? I'm not sure)
I tried to repeat search of insert position performed by `FastISel::recomputeInsertPt()`. Now tried to get misplaced debug locations without this change and couldn't, so removed it.

================
Comment at: lib/CodeGen/SelectionDAG/FastISel.cpp:142
@@ +141,3 @@
+  MachineBasicBlock::iterator InstWithLoc = FuncInfo.InsertPt;
+  while (InstWithLoc != FuncInfo.MBB->end() && !InstWithLoc->getDebugLoc())
+    ++InstWithLoc;
----------------
dblaikie wrote:
> Could use std::find_if here, if you like.
Right, that's better.

================
Comment at: test/DebugInfo/X86/dbg-line-fast-isel.ll:10
@@ +9,3 @@
+; {
+;   callme(100);
+;   if (arg > 5000)
----------------
dblaikie wrote:
> Do the three blocks in this function all test interesting parts of the patch? Or could we use two? If they're all interesting it might be worth a comment to explain how/why.
> 
> Do these test functions need non-void returns, or could they be simplified to return void?
> 
> (does the callme function need a non-void return too? Does it need a parameter? (I assume it needs the parameter for some reason))
> Do the three blocks in this function all test interesting parts of the patch? Or could we use two? If they're all interesting it might be worth a comment to explain how/why.

Not really, the first call isn't actually checked for anything, that check is in the second function.

> Do these test functions need non-void returns, or could they be simplified to return void?

The first one does, because there is a check that `0` from `return 0` has correct debug location.

> does the callme function need a non-void return too?

It's not necessary, will be changed.

> Does it need a parameter? (I assume it needs the parameter for some reason)

Yes, it's the parameter debug location we're worried about here, as it's a "local value" and code was changed to update location for local values.

http://reviews.llvm.org/D9887

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/