[PATCH] D83468: [Debuginfo] Fix for PR46653

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:107
 
-  MIRBuilder.setInstrAndDebugLoc(MI);
+  MIRBuilder.setInstr(MI);
 
----------------
Jac1494 wrote:
> arsenm wrote:
> > Jac1494 wrote:
> > > aprantl wrote:
> > > > This also looks wrong. We would be inheriting a random previous debug location for all instructions created by MIRBuilder if we did this?
> > > Yes, I have also check the debugging experience and now it is looks correct.
> > Pretty sure this is completely broken. The currently legalizing instruction is the only reasonable choice to set here
> If we don't set debug location here than also in executable debug location entry is added properly , because IRTransaltor is already setting debug location. I have tried test case given below and others test cases too and debugging experience is improved lot after this patch. 
> 
> ```
> int var1;           // 1
> int var2;           // 2
> int main() {        // 3
>   if (var1 == 1) {  // 4
>     var2 = 2;       // 5
>   }                 // 6
>   return 0;         // 7
> }
> ```
> 
> Also if we add debug location here than it is giving incorrect debugging experience because of line number 0 entry added into debugline table which is added from here ,than what is need of setting debug location here (In legalizer)...? 
> 
> >Pretty sure this is completely broken.
> Can you please explain more what is completely broken here..?
The IRTranslator added a debug location to the initial instruction.  The same debug location needs to be propagated to all instructions emitted when the instruction is legalized, which setting it here accomplishes. This is not setting or changing the debug location of the instruction itself, and only ensures that new instructions derived from it end up with the same location


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

https://reviews.llvm.org/D83468