Page MenuHomePhabricator

[MIR-Canon] Fixing case where MachineFunction is empty.
ClosedPublic

Authored by plotfi on May 29 2019, 11:56 AM.

Details

Summary

This small change is to handle changes where a machine function is:

body: |
...

Currently, dereferencing the MF.begin() prior to RPO causes core dump.

Diff Detail

Repository
rL LLVM

Event Timeline

plotfi created this revision.May 29 2019, 11:56 AM
arsenm added a subscriber: arsenm.May 29 2019, 12:07 PM
arsenm added inline comments.
llvm/test/CodeGen/MIR/AArch64/print-parse-overloaded-intrinsics.mir
5–6 ↗(On Diff #202012)

I don't see how the test is related?

plotfi marked an inline comment as done.May 29 2019, 12:27 PM
plotfi added inline comments.
llvm/test/CodeGen/MIR/AArch64/print-parse-overloaded-intrinsics.mir
5–6 ↗(On Diff #202012)

You have a good point. I will put together a separate test. This one just needs to be (empty MIR, but the IR part has a function):

--- |
  define i32 @foo() {
    ret i32 0
  }
...
bogner added inline comments.May 29 2019, 12:28 PM
llvm/lib/CodeGen/MIRCanonicalizerPass.cpp
107 ↗(On Diff #202012)

MF.empty()?

108 ↗(On Diff #202012)

Should be sufficient to write return {};

llvm/test/CodeGen/MIR/AArch64/print-parse-overloaded-intrinsics.mir
5–6 ↗(On Diff #202012)

Agreed, it's probably better to create a simple test for this explicitly, and you should probably FileCheck that it passes the empty block through unchanged

plotfi updated this revision to Diff 202080.May 29 2019, 5:09 PM

Changes based on Matt+Justin's feedback.

plotfi marked 2 inline comments as done.May 29 2019, 5:10 PM
plotfi marked 2 inline comments as done.
bogner added inline comments.May 30 2019, 11:40 AM
llvm/test/CodeGen/MIR/AArch64/empty-MF.mir
1–2 ↗(On Diff #202080)

Probably best to FileCheck that we get an empty function rather than just "not crashing"

2 ↗(On Diff #202080)

Why -verify-machineinstrs? There aren't any...

4–8 ↗(On Diff #202080)

This is kind of a confusing way to do this - the function certainly doesn't look empty and you're relying on the fact that not providing the mir gives us something empty-ish. I'd probably drop the IR completely and write out the mir you want:

---
name: foo
body:             |
...
plotfi marked an inline comment as done.May 30 2019, 11:43 AM
plotfi added inline comments.
llvm/test/CodeGen/MIR/AArch64/empty-MF.mir
4–8 ↗(On Diff #202080)

I will give this a try. This is what was crashing the other test though.

plotfi updated this revision to Diff 202300.May 30 2019, 2:13 PM
plotfi marked 3 inline comments as done.

Addressing Justin's latest feedback.

bogner accepted this revision.May 30 2019, 2:15 PM
This revision is now accepted and ready to land.May 30 2019, 2:15 PM
This revision was automatically updated to reflect the committed changes.