Page MenuHomePhabricator

llvm-reduce: Add pass to reduce MIR instruction flags
ClosedPublic

Authored by arsenm on Apr 19 2022, 2:15 PM.

Diff Detail

Event Timeline

arsenm created this revision.Apr 19 2022, 2:15 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 19 2022, 2:15 PM
Herald added a subscriber: mgorny. · View Herald Transcript
arsenm requested review of this revision.Apr 19 2022, 2:15 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 19 2022, 2:15 PM
Herald added a subscriber: wdng. · View Herald Transcript

Removing instruction flags seems dangerous to me:

  • Dropping BundledPred, BundledSucc will unbundle instructions and leave a bundle header around in the instruction stream, which seems bad.
  • I wouldn't be surprised if some targets break/segfault when FrameSetup/FrameDestroy is dropped randomly.

I could see this work okay for the math and overflow flags. Maybe it would be best to explicitely list those "mostly harmless" flags here and leave the others untouched?

llvm/tools/llvm-reduce/ReducerWorkItem.cpp
247

Nice catch.

MatzeB added inline comments.Apr 26 2022, 8:55 PM
llvm/tools/llvm-reduce/ReducerWorkItem.cpp
247

Feel free to just push this independently without separate review.

Removing instruction flags seems dangerous to me:

  • Dropping BundledPred, BundledSucc will unbundle instructions and leave a bundle header around in the instruction stream, which seems bad.
  • I wouldn't be surprised if some targets break/segfault when FrameSetup/FrameDestroy is dropped randomly.

I forgot these were stored as part of flags. However, I think I accidentally avoided this bug. setFlags actually does take care to preserve the BundledPred/BundledSucc:

void setFlags(unsigned flags) {
   // Filter out the automatically maintained flags.
   unsigned Mask = BundledPred | BundledSucc;
   Flags = (Flags & Mask) | (flags & ~Mask);
 }

I guess there still is potentially a bug from assuming getFlags != 0 can be reduced further, although it should at worst cause an avoidable reduction attempt.
As far as FrameSetup/FrameDestroy are concerned, it's a bug if the targets break on them. Unless there's a verifier enforced property, targets should be able to handle these unset

llvm/tools/llvm-reduce/ReducerWorkItem.cpp
247

Also forgot about AsmPrinterFlags

MatzeB accepted this revision.May 18 2022, 2:02 PM

As far as FrameSetup/FrameDestroy are concerned, it's a bug if the targets break on them.

You can probably debate that with target authors :)... Anyway I don't have strong feelings and this seems valuable to move forward with.

LGTM

This revision is now accepted and ready to land.May 18 2022, 2:02 PM

As far as FrameSetup/FrameDestroy are concerned, it's a bug if the targets break on them.

You can probably debate that with target authors :)... Anyway I don't have strong feelings and this seems valuable to move forward with.

If there's some required property, it would need to be checked by the verifier. If it's not checked by the verifier, you can't rely on it. Cases that fail the verifier are filtered out