Page MenuHomePhabricator

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

Authored by asb on Aug 24 2017, 1:20 AM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

kparzysz edited edge metadata.Aug 24 2017, 9:08 AM

The Hexagon changes look good to me.

qcolombet added inline comments.Aug 24 2017, 10:47 AM
lib/CodeGen/MachineLICM.cpp
859 ↗(On Diff #112513)

I would rather change isSafeToMove to return false for PHIs.

asb updated this revision to Diff 112584.Aug 24 2017, 11:09 AM
asb marked an inline comment as done.
asb edited the summary of this revision. (Show Details)

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

This revision is now accepted and ready to land.Aug 24 2017, 4:59 PM
This revision was automatically updated to reflect the committed changes.