This is an archive of the discontinued LLVM Phabricator instance.

[MIRPrinter] Add machine metadata support.
ClosedPublic

Authored by hliao on May 26 2021, 1:44 PM.

Details

Summary
  • Distinct metadata needs generating in the codegen to attach correct AAInfo on the loads/stores after lowering, merging, and other relevant transformations.
  • This patch adds 'MachhineModuleSlotTracker' to help assign slot numbers to these newly generated unnamed metadata nodes.
  • To help 'MachhineModuleSlotTracker' track machine metadata, the original 'SlotTracker' is rebased from 'AbstractSlotTrackerStorage', which provides basic interfaces to create/retrive metadata slots. In addition, once LLVM IR is processsed, additional hooks are also introduced to help collect machine metadata and assign them slot numbers.
  • Finally, if there is any such machine metadata, 'MIRPrinter' outputs an additional 'machineMetadataNodes' field containing all the definition of those nodes.

Diff Detail

Event Timeline

hliao created this revision.May 26 2021, 1:44 PM
hliao requested review of this revision.May 26 2021, 1:44 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 26 2021, 1:44 PM
hliao updated this revision to Diff 348344.May 27 2021, 11:50 AM

Revise that machine tracker a bit to ensure machine metadata is alway per-function state.

  • The printer of the machine function is done on a per-function wise. That makes the scanning of machine data through module unnecessary and problematic as MF may be removed deliberately under certain cases. I found that issue when validating the parser support.
nikic edited reviewers, added: arsenm; removed: nikic.May 27 2021, 11:57 AM
nikic added a subscriber: arsenm.

Unfortunately I don't know anything about MIR -- adding @arsenm who I think should be familiar with this, or at least know someone who is :)

Thanks for doing this, this has been painful forever.

Can you add some plain mir tests in test/CodeGen/MIR?

llvm/include/llvm/CodeGen/MIRYamlMapping.h
707

Is this just for the metadata nodes attached to the function itself?

llvm/unittests/CodeGen/MachineMetadata.cpp
123 ↗(On Diff #348344)

EXPECT_EQ

155 ↗(On Diff #348344)

EXPECT_EQ

327–328 ↗(On Diff #348344)

Only need the hasOneMemOperand

hliao updated this revision to Diff 348376.May 27 2021, 1:46 PM

Separate that test into MIR.

hliao marked 3 inline comments as done.May 27 2021, 1:55 PM

the patch is revised following comments.

llvm/include/llvm/CodeGen/MIRYamlMapping.h
707

yes, as 'printMIR' inherently works per function wise, the metadata here refers to ones attached to the specified MF. So far, metadata generated in the backend are all unnamed ones and only referenced within that function only.
Furthermore, our MIR/MF in some tools may be removed in a function-pass ('createFreeMachineFunctionPass'). That makes the whole machine module inconsistent between prints of different MFs.

hliao updated this revision to Diff 348393.May 27 2021, 2:38 PM

Fix warngins from clang-tidy.

arsenm added inline comments.May 27 2021, 6:18 PM
llvm/include/llvm/CodeGen/MIRYamlMapping.h
707

So the intent is you do not need the IR section to use the metadata? I had assumed this would directly read from the IR. I'm all for breaking the IR dependency but wondering what happens if the IR section metadata is inconsistent with the MIR section metadata

hliao added inline comments.May 28 2021, 8:50 AM
llvm/include/llvm/CodeGen/MIRYamlMapping.h
707

those metadata is generated directly within the backend instead of reading from IR. For instance, in D102255, new scoped AA metadata is generated when lowering memcpy into machine instructions. In D102821, when two stores are merged, potentially we need a new scoped AA metadata as the new scope should be the union of that 2 stores and the new scoped no-alias should be the intersect. In short, with the newly introduced scoped AA metadata, the backend may need to generate new metadata, which could not be referenced directly from IR.

hliao added a comment.Jun 1 2021, 9:15 AM

Kindly PING

hliao added a comment.Jun 9 2021, 6:45 AM

Kindly PING again

arsenm accepted this revision.Jun 10 2021, 3:14 PM
This revision is now accepted and ready to land.Jun 10 2021, 3:14 PM
arsenm requested changes to this revision.Jun 10 2021, 3:16 PM

Needs standalone MIR tests (i.e. round trip tests not bundled in a unit test)

Also I would like to see some tests where there are inconsistencies between the metadata present in the IR section, and the explicit metadata in the MIR

This revision now requires changes to proceed.Jun 10 2021, 3:16 PM
hliao added a comment.Jun 15 2021, 7:44 AM

Needs standalone MIR tests (i.e. round trip tests not bundled in a unit test)

Also I would like to see some tests where there are inconsistencies between the metadata present in the IR section, and the explicit metadata in the MIR

round-trip needs parser support (under review @ https://reviews.llvm.org/D103282), where cases with mixed metadata are also included. Shall we merge them into one?

PING for the further comment on the round-trip test, which requires MIR parser support reviewed @ D103282

arsenm accepted this revision.Jun 18 2021, 3:42 PM
This revision is now accepted and ready to land.Jun 18 2021, 3:42 PM
This revision was landed with ongoing or failed builds.Jun 19 2021, 9:48 AM
This revision was automatically updated to reflect the committed changes.