[PATCH] D37097: Set hasSideEffects=0 for PHI and fix passes relying isSafeToMove/hasUnmodeledSideEffects being true for PHI

[Alex Bradbury via Phabricator via llvm-commits]

asb created this revision.

Previously, hasSideEffects was ? for TargetOpcode::PHI and would be inferred as 1. https://reviews.llvm.org/D37065 sets the current inferred properties explicitly. This patch sets hasSideEffects=0 for PHI, matching G_PHI. After making this change, MachineInstr::isSafeToMove will no longer return false for a PHI node, causing issues in the MachineLICM and MachineSink passes. I have fixed these passes to check MI->isPHI().

Additionally, HexagonBitSimplify relied on a PHI node having the hasUnmodeledSideEffects property. @kparzysz, does the fix look correct here? Looking at the Hexagon passes, it's probably worth someone familiar with that code auditing code paths that use hasUnmodeledSideEffects or isSafeToMove. e.g. in RDFDeadCode, isLiveInstr will now return false for a PHI instruction rather than true (it would previously have only returned false for a G_PHI).

All unit tests pass with this change.


https://reviews.llvm.org/D37097

Files:
  include/llvm/Target/Target.td
  lib/CodeGen/MachineInstr.cpp
  lib/Target/Hexagon/HexagonBitSimplify.cpp


Index: lib/Target/Hexagon/HexagonBitSimplify.cpp
===================================================================
--- lib/Target/Hexagon/HexagonBitSimplify.cpp
+++ lib/Target/Hexagon/HexagonBitSimplify.cpp
@@ -1319,7 +1319,7 @@
 
     if (MI->getOpcode() == TargetOpcode::COPY)
       continue;
-    if (MI->hasUnmodeledSideEffects() || MI->isInlineAsm())
+    if (MI->isPHI() || MI->hasUnmodeledSideEffects() || MI->isInlineAsm())
       continue;
     unsigned NumD = MI->getDesc().getNumDefs();
     if (NumD != 1)
@@ -1329,8 +1329,7 @@
     if (!BT.has(RD.Reg))
       continue;
     const BitTracker::RegisterCell &DC = BT.lookup(RD.Reg);
-    auto At = MI->isPHI() ? B.getFirstNonPHI()
-                          : MachineBasicBlock::iterator(MI);
+    auto At = MachineBasicBlock::iterator(MI);
 
     // Find a source operand that is equal to the result.
     for (auto &Op : MI->uses()) {
Index: lib/CodeGen/MachineInstr.cpp
===================================================================
--- lib/CodeGen/MachineInstr.cpp
+++ lib/CodeGen/MachineInstr.cpp
@@ -1636,7 +1636,7 @@
   // Treat volatile loads as stores. This is not strictly necessary for
   // volatiles, but it is required for atomic loads. It is not allowed to move
   // a load across an atomic load with Ordering > Monotonic.
-  if (mayStore() || isCall() ||
+  if (mayStore() || isCall() || isPHI() ||
       (mayLoad() && hasOrderedMemoryRef())) {
     SawStore = true;
     return false;
Index: include/llvm/Target/Target.td
===================================================================
--- include/llvm/Target/Target.td
+++ include/llvm/Target/Target.td
@@ -815,7 +815,7 @@
   let OutOperandList = (outs unknown:$dst);
   let InOperandList = (ins variable_ops);
   let AsmString = "PHINODE";
-  let hasSideEffects = 1;
+  let hasSideEffects = 0;
 }
 def INLINEASM : Instruction {
   let OutOperandList = (outs);


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D37097.112509.patch
Type: text/x-patch
Size: 1904 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170824/dc066a61/attachment.bin>