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

[Matthias Braun via Phabricator via llvm-commits]

MatzeB accepted this revision.
MatzeB added a comment.
This revision is now accepted and ready to land.

Added some suggestions. Either way LGTM



================
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:
> 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?


================
Comment at: llvm/lib/Target/X86/X86InstrInfo.cpp:1423-1424
     NewMI = addOffset(MIB, 1);
+    if (LV && SrcReg != Src.getReg())
+      LV->getVarInfo(SrcReg).Kills.push_back(NewMI);
     break;
----------------
as above


================
Comment at: llvm/lib/Target/X86/X86InstrInfo.cpp:1446-1447
     NewMI = addOffset(MIB, -1);
-
+    if (LV && SrcReg != Src.getReg())
+      LV->getVarInfo(SrcReg).Kills.push_back(NewMI);
     break;
----------------
as above


================
Comment at: llvm/lib/Target/X86/X86InstrInfo.cpp:1496-1499
+      if (SrcReg2 != Src2.getReg())
+        LV->getVarInfo(SrcReg2).Kills.push_back(NewMI);
+      if (SrcReg != SrcReg2 && SrcReg != Src.getReg())
+        LV->getVarInfo(SrcReg).Kills.push_back(NewMI);
----------------
Shouldn't this just correspond to the kill flags used in line 1494?


================
Comment at: llvm/lib/Target/X86/X86InstrInfo.cpp:1540
     NewMI = addOffset(MIB, MI.getOperand(2));
+    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