[PATCH] D133631: [X86] Fix the LEA optimization pass

[Kazu Hirata via Phabricator via llvm-commits]

kazu created this revision.
Herald added subscribers: pengfei, hiraditya.
Herald added a project: All.
kazu requested review of this revision.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

The LEA optimization pass visits each basic block of a given machine
function.  In each basic block, for each pair of LEAs that differ only
in their displacement fields, we replace all uses of the second LEA
with the first LEA while adjusting the displacement.

Now, without this patch, after all the replacements are made, the
following assert triggers:

  assert(MRI->use_empty(LastVReg) &&
         "The LEA's def register must have no uses");

The replacement loop uses:

  for (MachineOperand &MO :
       llvm::make_early_inc_range(MRI->use_operands(LastVReg))) {

which is equivalent to:

  for (auto UI = MRI->use_begin(LastVReg), UE = MRI->use_end();
       UI != UE;) {
    MachineOperand &MO = *UI++;  // <-- Look!

That is, immediately after the post increment, make_early_inc_range
already has the iterator for the next iteration in its mind.

The problem is that in one iteration of the loop, we could replace two
uses in a debug instruction like:

  DBG_VALUE_LIST !"r", !DIExpression(DW_OP_LLVM_arg, 0), %0:gr64, %0:gr64, ...

So, the iterator for the next iteration becomes invalid.  We end up
traversing a garbage use list from that point on.  In turn, we don't
get to visit remaining uses.

The patch fixes the problem by switching to a "draining" while loop:

  while (!MRI->use_empty(LastVReg)) {
    MachineOperand &MO = *MRI->use_begin(LastVReg);
    MachineInstr &MI = *MO.getParent();

I would really like to attach a testcase for this fix, but I haven't
come up with a suitable reduced testcase.  My semi-reduced .ll test
case still has 129 lines, including a lot of debug meta data, and it
contains a lot of seemingly unrelated code just to set up the two LEAs
along with the aforementioned debug instruction.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D133631

Files:
  llvm/lib/Target/X86/X86OptimizeLEAs.cpp


Index: llvm/lib/Target/X86/X86OptimizeLEAs.cpp
===================================================================
--- llvm/lib/Target/X86/X86OptimizeLEAs.cpp
+++ llvm/lib/Target/X86/X86OptimizeLEAs.cpp
@@ -653,8 +653,12 @@
         // isReplaceable function.
         Register FirstVReg = First.getOperand(0).getReg();
         Register LastVReg = Last.getOperand(0).getReg();
-        for (MachineOperand &MO :
-             llvm::make_early_inc_range(MRI->use_operands(LastVReg))) {
+        // We use use_empty here instead of the combination of
+        // make_early_inc_range and use_operands because we could replace two or
+        // more uses in a debug instruction in one iteration and that would
+        // deeply confuse make_early_inc_range.
+        while (!MRI->use_empty(LastVReg)) {
+          MachineOperand &MO = *MRI->use_begin(LastVReg);
           MachineInstr &MI = *MO.getParent();
 
           if (MI.isDebugValue()) {


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D133631.459253.patch
Type: text/x-patch
Size: 947 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20220910/24b599f7/attachment.bin>