[PATCH] D86676: [GISel]: Fix one more CSE Non determinism

[Aditya Nandakumar via Phabricator via llvm-commits]

aditya_nandakumar created this revision.
aditya_nandakumar added reviewers: arsenm, aemerson, bogner, dsanders, volkan.
Herald added subscribers: mgrang, hiraditya.
Herald added a project: LLVM.
aditya_nandakumar requested review of this revision.
Herald added a subscriber: wdng.

Sometimes we can have the following code

  x:gpr(s32) = G_OP

Say we build G_OP2 to the same x and then delete the previous instruction. Using something like

  Register X = ...;
  auto NewMIB = CSEBuilder.buildOp2(X, ... args);

Currently there's a mismatch in how NewMIB is profiled and inserted into the CSEMap (ie it doesn't consider register bank/register class along with type).Unify the profiling by refactoring and calling the common method.

This was found by turning on the CSEInfo::verify in at the end of each of our GISel passes which turns inconsistent state/non determinism in CSEing into crashes which likely usually indicates missing calls to Observer on mutations (the most common case). Here non determinism usually means not cseing sometimes, but almost never about producing incorrect code.
Also this patch adds this verification at the end of the combiners as well.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D86676

Files:
  llvm/include/llvm/CodeGen/GlobalISel/CSEInfo.h
  llvm/lib/CodeGen/GlobalISel/CSEInfo.cpp
  llvm/lib/CodeGen/GlobalISel/CSEMIRBuilder.cpp
  llvm/lib/CodeGen/GlobalISel/Combiner.cpp


Index: llvm/lib/CodeGen/GlobalISel/Combiner.cpp
===================================================================
--- llvm/lib/CodeGen/GlobalISel/Combiner.cpp
+++ llvm/lib/CodeGen/GlobalISel/Combiner.cpp
@@ -153,5 +153,8 @@
     MFChanged |= Changed;
   } while (Changed);
 
+  assert(!CSEInfo || !errorToBool(CSEInfo->verify()) &&
+                         "CSEInfo is not consistent. Likely missing calls to "
+                         "observer on mutations");
   return MFChanged;
 }
Index: llvm/lib/CodeGen/GlobalISel/CSEMIRBuilder.cpp
===================================================================
--- llvm/lib/CodeGen/GlobalISel/CSEMIRBuilder.cpp
+++ llvm/lib/CodeGen/GlobalISel/CSEMIRBuilder.cpp
@@ -62,6 +62,11 @@
   case DstOp::DstType::Ty_RC:
     B.addNodeIDRegType(Op.getRegClass());
     break;
+  case DstOp::DstType::Ty_Reg: {
+    // Regs can have LLT&(RB|RC). If those exist, profile them as well.
+    B.addNodeIDReg(Op.getReg());
+    break;
+  }
   default:
     B.addNodeIDRegType(Op.getLLTTy(*getMRI()));
     break;
Index: llvm/lib/CodeGen/GlobalISel/CSEInfo.cpp
===================================================================
--- llvm/lib/CodeGen/GlobalISel/CSEInfo.cpp
+++ llvm/lib/CodeGen/GlobalISel/CSEInfo.cpp
@@ -367,23 +367,30 @@
   return *this;
 }
 
+const GISelInstProfileBuilder &
+GISelInstProfileBuilder::addNodeIDReg(Register Reg) const {
+  LLT Ty = MRI.getType(Reg);
+  if (Ty.isValid())
+    addNodeIDRegType(Ty);
+
+  if (const RegClassOrRegBank &RCOrRB = MRI.getRegClassOrRegBank(Reg)) {
+    if (const auto *RB = RCOrRB.dyn_cast<const RegisterBank *>())
+      addNodeIDRegType(RB);
+    else if (const auto *RC = RCOrRB.dyn_cast<const TargetRegisterClass *>())
+      addNodeIDRegType(RC);
+  }
+  return *this;
+}
+
 const GISelInstProfileBuilder &GISelInstProfileBuilder::addNodeIDMachineOperand(
     const MachineOperand &MO) const {
   if (MO.isReg()) {
     Register Reg = MO.getReg();
     if (!MO.isDef())
       addNodeIDRegNum(Reg);
-    LLT Ty = MRI.getType(Reg);
-    if (Ty.isValid())
-      addNodeIDRegType(Ty);
-
-    if (const RegClassOrRegBank &RCOrRB = MRI.getRegClassOrRegBank(Reg)) {
-      if (const auto *RB = RCOrRB.dyn_cast<const RegisterBank *>())
-        addNodeIDRegType(RB);
-      else if (const auto *RC = RCOrRB.dyn_cast<const TargetRegisterClass *>())
-        addNodeIDRegType(RC);
-    }
 
+    // Profile the register properties.
+    addNodeIDReg(Reg);
     assert(!MO.isImplicit() && "Unhandled case");
   } else if (MO.isImm())
     ID.AddInteger(MO.getImm());
Index: llvm/include/llvm/CodeGen/GlobalISel/CSEInfo.h
===================================================================
--- llvm/include/llvm/CodeGen/GlobalISel/CSEInfo.h
+++ llvm/include/llvm/CodeGen/GlobalISel/CSEInfo.h
@@ -182,6 +182,8 @@
 
   const GISelInstProfileBuilder &addNodeIDRegNum(Register Reg) const;
 
+  const GISelInstProfileBuilder &addNodeIDReg(Register Reg) const;
+
   const GISelInstProfileBuilder &addNodeIDImmediate(int64_t Imm) const;
   const GISelInstProfileBuilder &
   addNodeIDMBB(const MachineBasicBlock *MBB) const;


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D86676.288179.patch
Type: text/x-patch
Size: 3107 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200827/f4601b7a/attachment.bin>