[PATCH] D129634: [X86] Fix updating LiveVariables in convertToThreeAddress

[Matthias Braun via Phabricator via llvm-commits]

MatzeB added inline comments.


================
Comment at: llvm/lib/Target/X86/X86InstrInfo.cpp:1390-1391
     NewMI = MIB;
-
+    if (LV && SrcReg != Src.getReg())
+      LV->getVarInfo(SrcReg).Kills.push_back(NewMI);
     break;
----------------
foad wrote:
> MatzeB wrote:
> > foad wrote:
> > > I'm not proud of this patch because it adds duplicated code like this after almost every call to classifyLEAReg. There may be better ways to do this, by completely rethinking the way that convertToThreeAddress updates LiveVariables.
> > > 
> > > On the other hand LiveVariables is supposed to go away in favour of LiveIntervals, so does it really matter?
> > Can't we just use the already computed `isKill` here?
> Yes this works. I didn't do it that way because it will add duplicates in the Kills lists, because the call to replaceKillInstruction below will also add them (in the case that SrcReg == Src.getReg(), i.e. classifyLEAReg did not create a new register), but that does not seem to cause any problems in practice. What do you prefer?
I see, makes sense. Maybe hint at it with a comment then?

```
// Record kill in cases where we don't call `replaceKillInstruction` later.
if (LV && SrcReg != Src.getReg())
  LV->getVarInfo(SrcReg).Kills.push_back(NewMI);
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129634