MachineInstr: Make isEqual agree with getHashValue in MachineInstrExpressionTrait

[rovka]

Summary

MachineInstr::isIdenticalTo has a lot of logic for dealing with register
Defs (i.e. deciding whether to take them into account or ignore them).
This logic gets things wrong in some obscure cases, for instance if an
operand is not a Def for both the current MI and the one we are comparing to.

I'm not sure if it's possible for this to happen for regular register
operands, but it may happen in the ARM backend for special operands
which use sentinel values for the register (i.e. 0, which is neither a
physical register nor a virtual one).

This causes MachineInstrExpressionTrait::isEqual (which uses
MachineInstr::isIdenticalTo) to return true for the following
instructions, which are the same except for the fact that one sets the
flags and the other one doesn't:
%1114 = ADDrsi %1113, %216, 17, 14, _, def _
%1115 = ADDrsi %1113, %216, 17, 14, _, _

OTOH, MachineInstrExpressionTrait::getHashValue returns different
values for the 2 instructions due to the different isDef on the last
operand. In practice this means that when trying to add those
instructions to a DenseMap, they will be considered different because of
their different hash values, but when growing the map we might get an
assertion while copying from the old buckets to the new buckets because
isEqual misleadingly returns true.

This patch makes sure that isEqual and getHashValue agree, by improving
the checks in MachineInstr::isIdenticalTo when we are ignoring virtual
register definitions (which is what the Trait uses). Firstly, instead of checking
isPhysicalRegister, we use !isVirtualRegister, so that we cover both physical
registers and sentinel values. Secondly, instead of checking
MachineOperand::isReg, we use MachineOperand::isIdenticalTo, which
checks isReg, isSubReg and isDef, which are the same values that the
hash function uses to compute the hash.

Note that the function is symmetric with this change, since if the
current operand is not a Def, we check MachineOperand::isIdenticalTo,
which returns false if the operands have different isDef's.

[rovka]

This is a latent issue that triggers on one of the buildbots if the planets align. I'm holding one of Sanjay's InstCombine patches (r314698) hostage because of this bug, so please have a look. Thanks!

[kparzysz]

Here's a comment from include/llvm/CodeGen/MachineInstr.h (definition of MachineInstrExpressionTrait)

/// Special DenseMapInfo traits to compare MachineInstr* by *value* of the
/// instruction rather than by pointer value.
/// The hashing and equality testing functions ignore definitions so this is
/// useful for CSE, etc.

The isEqual function explicitly requests IgnoreVRegDefs, supposedly on the basis that getHashValue would do the same. Your observation is that this is not the case. Assuming that this comment was true at some point, it may be worthwhile to check how the deviation was introduced.

[rovka]

In D38789#894648, @kparzysz wrote:

Here's a comment from include/llvm/CodeGen/MachineInstr.h (definition of MachineInstrExpressionTrait)

/// Special DenseMapInfo traits to compare MachineInstr* by *value* of the
/// instruction rather than by pointer value.
/// The hashing and equality testing functions ignore definitions so this is
/// useful for CSE, etc.

The isEqual function explicitly requests IgnoreVRegDefs, supposedly on the basis that getHashValue would do the same. Your observation is that this is not the case. Assuming that this comment was true at some point, it may be worthwhile to check how the deviation was introduced.

Well, they ignore "proper" definitions (e.g. the %1114 vs %1115 in my example). That's a def on both instructions. But the last operand is only a def in one of the instructions, not both, so it's less clear what should happen. I tend to agree with the hashing function, which says that they are different.

Now that you mention it, I suppose they only need to be different if Check == IgnoreVRegDefs, since that is what the hashing function assumes, and only for non-virtual registers (physical and sentries). I'll update the patch to that effect.

I think in light of that idea, we should have some tests making sure that MachineInstrExpressionTrait::isEqual and getHashValue agree about things (independent of the tests for MachineInstr::isIdenticalTo).

[rovka]

Update to take into account isDef only when ignoring virtual defs, since this is what the getHashValue function assumes.
First of all, this changes from using isPhysicalRegister to using !isVirtualRegisters, which also includes the sentinel values that we're interested in.
Secondly, it changes from checking MO.getReg to checking MO.isIdenticalTo, which takes into account MO.getReg, MO.getSubReg and MO.isDef. These are the same things that the hash function combines when getting the hashes.

[rovka]

Add unit tests and update summary before commit.

llvm/trunk/lib/CodeGen/MachineInstr.cpp

for (unsigned i = 0, e = getNumOperands(); i != e; ++i) {
 
     // Clients may or may not want to ignore defs when testing for equality.
     // For example, machine CSE pass only cares about finding common
     // subexpressions, so it's safe to ignore virtual register defs.
     if (MO.isDef()) {
       if (Check == IgnoreDefs)
         continue;
       else if (Check == IgnoreVRegDefs) {
-        if (TargetRegisterInfo::isPhysicalRegister(MO.getReg()) ||
-            TargetRegisterInfo::isPhysicalRegister(OMO.getReg()))
-          if (MO.getReg() != OMO.getReg())
+        if (!TargetRegisterInfo::isVirtualRegister(MO.getReg()) ||
+            !TargetRegisterInfo::isVirtualRegister(OMO.getReg()))
+          if (!MO.isIdenticalTo(OMO))
             return false;
       } else {
         if (!MO.isIdenticalTo(OMO))
           return false;
         if (Check == CheckKillDead && MO.isDead() != OMO.isDead())
           return false;
       }
     } else {

llvm/trunk/unittests/CodeGen/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   AsmPrinter
   CodeGen
   Core
+  MC
+  SelectionDAG
   Support
+  Target
   )
 
 set(CodeGenSources
   DIEHashTest.cpp
   LowLevelTypeTest.cpp
   MachineInstrBundleIteratorTest.cpp
+  MachineInstrTest.cpp
   MachineOperandTest.cpp
   ScalableVectorMVTsTest.cpp
   )
 
 add_llvm_unittest(CodeGenTests
   ${CodeGenSources}
   )
 
 add_subdirectory(GlobalISel)

llvm/trunk/unittests/CodeGen/MachineInstrTest.cpp

+//===- MachineInstrTest.cpp -----------------------------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "llvm/CodeGen/MachineInstr.h"
+#include "llvm/CodeGen/MachineFunction.h"
+#include "llvm/CodeGen/MachineModuleInfo.h"
+#include "llvm/Support/TargetRegistry.h"
+#include "llvm/Support/TargetSelect.h"
+#include "llvm/Target/TargetFrameLowering.h"
+#include "llvm/Target/TargetInstrInfo.h"
+#include "llvm/Target/TargetLowering.h"
+#include "llvm/Target/TargetMachine.h"
+#include "llvm/Target/TargetOptions.h"
+#include "llvm/Target/TargetSubtargetInfo.h"
+#include "gtest/gtest.h"
+
+using namespace llvm;
+
+namespace {
+// Add a few Bogus backend classes so we can create MachineInstrs without
+// depending on a real target.
+class BogusTargetLowering : public TargetLowering {
+public:
+  BogusTargetLowering(TargetMachine &TM) : TargetLowering(TM) {}
+};
+
+class BogusFrameLowering : public TargetFrameLowering {
+public:
+  BogusFrameLowering()
+      : TargetFrameLowering(TargetFrameLowering::StackGrowsDown, 4, 4) {}
+
+  void emitPrologue(MachineFunction &MF,
+                    MachineBasicBlock &MBB) const override {}
+  void emitEpilogue(MachineFunction &MF,
+                    MachineBasicBlock &MBB) const override {}
+  bool hasFP(const MachineFunction &MF) const override { return false; }
+};
+
+class BogusSubtarget : public TargetSubtargetInfo {
+public:
+  BogusSubtarget(TargetMachine &TM)
+      : TargetSubtargetInfo(Triple(""), "", "", {}, {}, nullptr, nullptr,
+                            nullptr, nullptr, nullptr, nullptr, nullptr),
+        FL(), TL(TM) {}
+  ~BogusSubtarget() override {}
+
+  const TargetFrameLowering *getFrameLowering() const override { return &FL; }
+
+  const TargetLowering *getTargetLowering() const override { return &TL; }
+
+  const TargetInstrInfo *getInstrInfo() const override { return &TII; }
+
+private:
+  BogusFrameLowering FL;
+  BogusTargetLowering TL;
+  TargetInstrInfo TII;
+};
+
+class BogusTargetMachine : public LLVMTargetMachine {
+public:
+  BogusTargetMachine()
+      : LLVMTargetMachine(Target(), "", Triple(""), "", "", TargetOptions(),
+                          Reloc::Static, CodeModel::Small, CodeGenOpt::Default),
+        ST(*this) {}
+  ~BogusTargetMachine() override {}
+
+  const TargetSubtargetInfo *getSubtargetImpl(const Function &) const override {
+    return &ST;
+  }
+
+private:
+  BogusSubtarget ST;
+};
+
+std::unique_ptr<BogusTargetMachine> createTargetMachine() {
+  return llvm::make_unique<BogusTargetMachine>();
+}
+
+std::unique_ptr<MachineFunction> createMachineFunction() {
+  LLVMContext Ctx;
+  Module M("Module", Ctx);
+  auto Type = FunctionType::get(Type::getVoidTy(Ctx), false);
+  auto F = Function::Create(Type, GlobalValue::ExternalLinkage, "Test", &M);
+
+  auto TM = createTargetMachine();
+  unsigned FunctionNum = 42;
+  MachineModuleInfo MMI(TM.get());
+
+  return llvm::make_unique<MachineFunction>(F, *TM, FunctionNum, MMI);
+}
+
+// This test makes sure that MachineInstr::isIdenticalTo handles Defs correctly
+// for various combinations of IgnoreDefs, and also that it is symmetrical.
+TEST(IsIdenticalToTest, DifferentDefs) {
+  auto MF = createMachineFunction();
+
+  unsigned short NumOps = 2;
+  unsigned char NumDefs = 1;
+  MCOperandInfo OpInfo[] = {
+      {0, 0, MCOI::OPERAND_REGISTER, 0},
+      {0, 1 << MCOI::OptionalDef, MCOI::OPERAND_REGISTER, 0}};
+  MCInstrDesc MCID = {
+      0, NumOps,  NumDefs, 0,      0, 1ULL << MCID::HasOptionalDef,
+      0, nullptr, nullptr, OpInfo, 0, nullptr};
+
+  // Create two MIs with different virtual reg defs and the same uses.
+  unsigned VirtualDef1 = -42; // The value doesn't matter, but the sign does.
+  unsigned VirtualDef2 = -43;
+  unsigned VirtualUse = -44;
+
+  auto MI1 = MF->CreateMachineInstr(MCID, DebugLoc());
+  MI1->addOperand(*MF, MachineOperand::CreateReg(VirtualDef1, /*isDef*/ true));
+  MI1->addOperand(*MF, MachineOperand::CreateReg(VirtualUse, /*isDef*/ false));
+
+  auto MI2 = MF->CreateMachineInstr(MCID, DebugLoc());
+  MI2->addOperand(*MF, MachineOperand::CreateReg(VirtualDef2, /*isDef*/ true));
+  MI2->addOperand(*MF, MachineOperand::CreateReg(VirtualUse, /*isDef*/ false));
+
+  // Check that they are identical when we ignore virtual register defs, but not
+  // when we check defs.
+  ASSERT_FALSE(MI1->isIdenticalTo(*MI2, MachineInstr::CheckDefs));
+  ASSERT_FALSE(MI2->isIdenticalTo(*MI1, MachineInstr::CheckDefs));
+
+  ASSERT_TRUE(MI1->isIdenticalTo(*MI2, MachineInstr::IgnoreVRegDefs));
+  ASSERT_TRUE(MI2->isIdenticalTo(*MI1, MachineInstr::IgnoreVRegDefs));
+
+  // Create two MIs with different virtual reg defs, and a def or use of a
+  // sentinel register.
+  unsigned SentinelReg = 0;
+
+  auto MI3 = MF->CreateMachineInstr(MCID, DebugLoc());
+  MI3->addOperand(*MF, MachineOperand::CreateReg(VirtualDef1, /*isDef*/ true));
+  MI3->addOperand(*MF, MachineOperand::CreateReg(SentinelReg, /*isDef*/ true));
+
+  auto MI4 = MF->CreateMachineInstr(MCID, DebugLoc());
+  MI4->addOperand(*MF, MachineOperand::CreateReg(VirtualDef2, /*isDef*/ true));
+  MI4->addOperand(*MF, MachineOperand::CreateReg(SentinelReg, /*isDef*/ false));
+
+  // Check that they are never identical.
+  ASSERT_FALSE(MI3->isIdenticalTo(*MI4, MachineInstr::CheckDefs));
+  ASSERT_FALSE(MI4->isIdenticalTo(*MI3, MachineInstr::CheckDefs));
+
+  ASSERT_FALSE(MI3->isIdenticalTo(*MI4, MachineInstr::IgnoreVRegDefs));
+  ASSERT_FALSE(MI4->isIdenticalTo(*MI3, MachineInstr::IgnoreVRegDefs));
+}
+
+// Check that MachineInstrExpressionTrait::isEqual is symmetric and in sync with
+// MachineInstrExpressionTrait::getHashValue
+void checkHashAndIsEqualMatch(MachineInstr *MI1, MachineInstr *MI2) {
+  bool IsEqual1 = MachineInstrExpressionTrait::isEqual(MI1, MI2);
+  bool IsEqual2 = MachineInstrExpressionTrait::isEqual(MI2, MI1);
+
+  ASSERT_EQ(IsEqual1, IsEqual2);
+
+  auto Hash1 = MachineInstrExpressionTrait::getHashValue(MI1);
+  auto Hash2 = MachineInstrExpressionTrait::getHashValue(MI2);
+
+  ASSERT_EQ(IsEqual1, Hash1 == Hash2);
+}
+
+// This test makes sure that MachineInstrExpressionTraits::isEqual is in sync
+// with MachineInstrExpressionTraits::getHashValue.
+TEST(MachineInstrExpressionTraitTest, IsEqualAgreesWithGetHashValue) {
+  auto MF = createMachineFunction();
+
+  unsigned short NumOps = 2;
+  unsigned char NumDefs = 1;
+  MCOperandInfo OpInfo[] = {
+      {0, 0, MCOI::OPERAND_REGISTER, 0},
+      {0, 1 << MCOI::OptionalDef, MCOI::OPERAND_REGISTER, 0}};
+  MCInstrDesc MCID = {
+      0, NumOps,  NumDefs, 0,      0, 1ULL << MCID::HasOptionalDef,
+      0, nullptr, nullptr, OpInfo, 0, nullptr};
+
+  // Define a series of instructions with different kinds of operands and make
+  // sure that the hash function is consistent with isEqual for various
+  // combinations of them.
+  unsigned VirtualDef1 = -42;
+  unsigned VirtualDef2 = -43;
+  unsigned VirtualReg = -44;
+  unsigned SentinelReg = 0;
+  unsigned PhysicalReg = 45;
+
+  auto VD1VU = MF->CreateMachineInstr(MCID, DebugLoc());
+  VD1VU->addOperand(*MF,
+                    MachineOperand::CreateReg(VirtualDef1, /*isDef*/ true));
+  VD1VU->addOperand(*MF,
+                    MachineOperand::CreateReg(VirtualReg, /*isDef*/ false));
+
+  auto VD2VU = MF->CreateMachineInstr(MCID, DebugLoc());
+  VD2VU->addOperand(*MF,
+                    MachineOperand::CreateReg(VirtualDef2, /*isDef*/ true));
+  VD2VU->addOperand(*MF,
+                    MachineOperand::CreateReg(VirtualReg, /*isDef*/ false));
+
+  auto VD1SU = MF->CreateMachineInstr(MCID, DebugLoc());
+  VD1SU->addOperand(*MF,
+                    MachineOperand::CreateReg(VirtualDef1, /*isDef*/ true));
+  VD1SU->addOperand(*MF,
+                    MachineOperand::CreateReg(SentinelReg, /*isDef*/ false));
+
+  auto VD1SD = MF->CreateMachineInstr(MCID, DebugLoc());
+  VD1SD->addOperand(*MF,
+                    MachineOperand::CreateReg(VirtualDef1, /*isDef*/ true));
+  VD1SD->addOperand(*MF,
+                    MachineOperand::CreateReg(SentinelReg, /*isDef*/ true));
+
+  auto VD2PU = MF->CreateMachineInstr(MCID, DebugLoc());
+  VD2PU->addOperand(*MF,
+                    MachineOperand::CreateReg(VirtualDef2, /*isDef*/ true));
+  VD2PU->addOperand(*MF,
+                    MachineOperand::CreateReg(PhysicalReg, /*isDef*/ false));
+
+  auto VD2PD = MF->CreateMachineInstr(MCID, DebugLoc());
+  VD2PD->addOperand(*MF,
+                    MachineOperand::CreateReg(VirtualDef2, /*isDef*/ true));
+  VD2PD->addOperand(*MF,
+                    MachineOperand::CreateReg(PhysicalReg, /*isDef*/ true));
+
+  checkHashAndIsEqualMatch(VD1VU, VD2VU);
+  checkHashAndIsEqualMatch(VD1VU, VD1SU);
+  checkHashAndIsEqualMatch(VD1VU, VD1SD);
+  checkHashAndIsEqualMatch(VD1VU, VD2PU);
+  checkHashAndIsEqualMatch(VD1VU, VD2PD);
+
+  checkHashAndIsEqualMatch(VD2VU, VD1SU);
+  checkHashAndIsEqualMatch(VD2VU, VD1SD);
+  checkHashAndIsEqualMatch(VD2VU, VD2PU);
+  checkHashAndIsEqualMatch(VD2VU, VD2PD);
+
+  checkHashAndIsEqualMatch(VD1SU, VD1SD);
+  checkHashAndIsEqualMatch(VD1SU, VD2PU);
+  checkHashAndIsEqualMatch(VD1SU, VD2PD);
+
+  checkHashAndIsEqualMatch(VD1SD, VD2PU);
+  checkHashAndIsEqualMatch(VD1SD, VD2PD);
+
+  checkHashAndIsEqualMatch(VD2PU, VD2PD);
+}
+} // end namespace