Page MenuHomePhabricator

[SCCP][FuncSpec] Switch away from Instruction::isSafeToRemove()
Needs ReviewPublic

Authored by kpn on Jan 27 2022, 9:22 AM.

Details

Summary

In D115737 I found that I needed to teach Instruction::isSafeToRemove() about strictfp/constrained intrinsics. It was pointed out that this is probably the wrong function to use isInstructionTriviallyDead(). It doesn't make sense to have a "second, worse implementation".

I also believe that the Instruction class is the wrong place for this functionality. The information about whether or not an instruction can be removed is in the transform passes and should stay there.

I do not understand this pass well enough to add constrained support at this time, but this at least gets us away from the problematic API.

Diff Detail

Build Status
Buildable 188448

Event Timeline

kpn created this revision.Jan 27 2022, 9:22 AM
kpn requested review of this revision.Jan 27 2022, 9:22 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 27 2022, 9:22 AM
ormris removed a subscriber: ormris.Jan 27 2022, 10:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 21 2022, 10:51 AM
Herald added a subscriber: StephenFan. · View Herald Transcript
kpn updated this revision to Diff 440718.Jun 28 2022, 12:11 PM

Move more of Instruction::isSafeToRemove() into this function. The remainder of isSafeToRemove() isn't needed because we filter out function calls at the top of the changed function.

Can we use wouldInstructionBeTriviallyDead here as well? Relying on the CallBase bailout will probably come back bite us (at least, it's not really obvious to me why it is needed).

kpn updated this revision to Diff 462552.Sep 23 2022, 11:02 AM

Update for review comment: just use wouldInstructionBeTriviallyDead().

We did the same in IPSCCP a while ago D118387 and the verdict was to use the following:

static bool canRemoveInstruction(Instruction *I) {
  if (wouldInstructionBeTriviallyDead(I))
    return true;

  // Some instructions can be handled but are rejected above. Catch
  // those cases by falling through to here.
  // TODO: Mark globals as being constant earlier, so
  // TODO: wouldInstructionBeTriviallyDead() knows that atomic loads
  // TODO: are safe to remove.
  return isa<LoadInst>(I);
}
kpn added a comment.Sep 23 2022, 12:42 PM

D118387 was my ticket, and it was handled that way to preserve the existing behavior. I don't believe a wrapper is needed in this case.