Set hasSideEffects=0 for PHI and fix passes relying isSafeToMove/hasUnmodeledSideEffects being true for PHI

[asb]

Summary

Previously, hasSideEffects was ? for TargetOpcode::PHI and would be inferred as 1. D37065 sets the current inferred properties explicitly. This patch sets hasSideEffects=0 for PHI, as it is for G_PHI. MachineInstr::isSafeToMove has been updated so it still returns false for PHI.

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.

[asb]

Attach the correct patch.

[kparzysz]

The Hexagon changes look good to me.

[qcolombet]

lib/CodeGen/MachineLICM.cpp

859 ↗

(On Diff #112513)

I would rather change isSafeToMove to return false for PHIs.

[asb]

Update to have MachineInstr::isSafeToMove return false for PHI, as requested by @qcolombet.

[qcolombet]

LGTM

llvm/trunk/include/llvm/Target/Target.td

 // targets that set guessInstructionProperties=0. Any local definition of
 // mayLoad/mayStore takes precedence over these default values.
 let mayLoad = 0, mayStore = 0, isCodeGenOnly = 1, isPseudo = 1,
     hasNoSchedulingInfo = 1, Namespace = "TargetOpcode" in {
 def PHI : Instruction {
   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);
   let InOperandList = (ins variable_ops);
   let AsmString = "";
   let hasSideEffects = 0;  // Note side effect is encoded in an operand.
 }
 def CFI_INSTRUCTION : Instruction {

llvm/trunk/lib/CodeGen/MachineInstr.cpp

 /// SawStore is set to true, it means that there is a store (or call) between
 /// the instruction's location and its intended destination.
 bool MachineInstr::isSafeToMove(AliasAnalysis *AA, bool &SawStore) const {
   // Ignore stuff that we obviously can't move.
   //
   // 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;
   }
 
   if (isPosition() || isDebugValue() || isTerminator() ||
       hasUnmodeledSideEffects())
     return false;

llvm/trunk/lib/Target/Hexagon/HexagonBitSimplify.cpp

bool RedundantInstrElimination::processBlock(MachineBasicBlock &B,
   bool Changed = false;
 
   for (auto I = B.begin(), E = B.end(), NextI = I; I != E; ++I) {
     NextI = std::next(I);
     MachineInstr *MI = &*I;
 
     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)
       continue;
 
     BitTracker::RegisterRef RD = MI->getOperand(0);
     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()) {
       if (!Op.isReg())
         continue;
       BitTracker::RegisterRef RS = Op;
       if (!BT.has(RS.Reg))
         continue;