[PATCH] D38789: MachineInstr: Force isDef to match in isIdenticalTo

[Diana Picus via Phabricator via llvm-commits]

rovka created this revision.
Herald added subscribers: kristof.beyls, inglorion, mehdi_amini, wdng, aemerson.

MachineInstr::isIdenticalTo has a lot of logic for dealing with register
Defs (i.e. deciding whether to take them into account or ignore them). I
think this logic should only apply if an operand is a Def for both the
current MI and the one we are comparing to. If one of the instructions
is defining the register and the other one is only using it, then they
have sufficiently different behaviour for the instructions to be
considered different regardless of whether or not we are ignoring Defs
(since technically one of them is not a Def to begin with).

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 MachineInstrExpressionTraits::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, MachineInstrExpressionTraits::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.

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.

PS: I'm going to add a unit test for this, since the issue is difficult to 
reproduce otherwise, but I'd like to get some feedback on the
approach first.


https://reviews.llvm.org/D38789

Files:
  lib/CodeGen/MachineInstr.cpp


Index: lib/CodeGen/MachineInstr.cpp
===================================================================
--- lib/CodeGen/MachineInstr.cpp
+++ lib/CodeGen/MachineInstr.cpp
@@ -1126,6 +1126,8 @@
     // For example, machine CSE pass only cares about finding common
     // subexpressions, so it's safe to ignore virtual register defs.
     if (MO.isDef()) {
+      if (!OMO.isDef())
+        return false;
       if (Check == IgnoreDefs)
         continue;
       else if (Check == IgnoreVRegDefs) {


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D38789.118565.patch
Type: text/x-patch
Size: 497 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20171011/3f1cbc68/attachment.bin>