This is an archive of the discontinued LLVM Phabricator instance.

[MachineOutliner] Make getOutliningType partially target-independent
ClosedPublic

Authored by duck-37 on May 5 2022, 8:39 PM.

Details

Summary

The motivation behind this patch is to unify some of the outliner logic across architectures. This looks nicer in general and makes fixing issues like this easier.
There are some notable changes here:

  1. isMetaInstruction() is used directly instead of checking for specific meta-instructions like IMPLICIT_DEF or KILL. This was already done in the RISC-V implementation, but other architectures still did hardcoded checks.
    • As an exception to this, CFI instructions are explicitly delegated to the target because RISC-V has different handling for those.
  2. isTargetIndex() checks are replaced with an assert; none of the architectures supported actually use MO_TargetIndex at this point in time.
  3. isCFIIndex() and isFI() checks are also replaced with asserts, since these operands should not exist in any context at this stage in the pipeline.

Diff Detail

Event Timeline

duck-37 created this revision.May 5 2022, 8:39 PM
duck-37 requested review of this revision.May 5 2022, 8:39 PM
duck-37 updated this revision to Diff 427527.May 5 2022, 8:50 PM

Comment clarification and typo fix.

duck-37 updated this revision to Diff 427660.May 6 2022, 9:20 AM

Rebase and add assertions for other operands that shouldn't exist at this point.

duck-37 edited the summary of this revision. (Show Details)May 6 2022, 9:21 AM
duck-37 edited the summary of this revision. (Show Details)May 6 2022, 9:23 AM

I'm not qualified for reviewing this patch, but it makes sense to me. Thanks for doing these changes. :-)

llvm/include/llvm/CodeGen/TargetInstrInfo.h
1944

We may need to change \p MI to \p MIT or modify param name.

llvm/lib/CodeGen/TargetInstrInfo.cpp
1458

"frame indices" could be excluded since we assert them now. :-)

duck-37 updated this revision to Diff 427889.May 7 2022, 12:39 PM
duck-37 marked 2 inline comments as done.

Rebase, update a few comments.

duck-37 updated this revision to Diff 493480.EditedJan 30 2023, 8:18 PM
duck-37 edited the summary of this revision. (Show Details)

Rebase to fix w/ new LLVM version. @paquette ping, this PR has been open for some time now and it's blocking the more important change (preventing XRay pseudo-opcodes from being outlined without needless code duplication)

duck-37 edited the summary of this revision. (Show Details)Jan 30 2023, 8:18 PM
duck-37 edited the summary of this revision. (Show Details)
luke957 removed a subscriber: luke957.Feb 2 2023, 11:31 PM
paquette accepted this revision.Feb 9 2023, 11:13 AM

This is nice, thank you.

This revision is now accepted and ready to land.Feb 9 2023, 11:13 AM
This revision was landed with ongoing or failed builds.Feb 9 2023, 11:35 AM
This revision was automatically updated to reflect the committed changes.

It looks like this commit introduced a new crash in llc when building for macos
Here is the link to the reproducer
https://godbolt.org/z/heqaE1jYa

Please take a look and revert if it needs a non-trivial fix

fhahn added a subscriber: fhahn.Mar 28 2023, 10:37 AM

It looks like this commit introduced a new crash in llc when building for macos
Here is the link to the reproducer
https://godbolt.org/z/heqaE1jYa

Please take a look and revert if it needs a non-trivial fix

Looking into it now, thanks.

It looks like this commit introduced a new crash in llc when building for macos
Here is the link to the reproducer
https://godbolt.org/z/heqaE1jYa

Please take a look and revert if it needs a non-trivial fix

Looking into it now, thanks.

I've found the issue. Making a reduced test-case now.

@zjaffal

most likely won't be able to get this done fully by tonight, so WIP patch is attached

@zjaffal

most likely won't be able to get this done fully by tonight, so WIP patch is attached

I tested the patch attached on the full project and it fixes the build failure.
Thanks