This is an archive of the discontinued LLVM Phabricator instance.

[TargetLowering] Add hook for adding target MMO flags when doing ISel.
ClosedPublic

Authored by gberry on Jul 3 2017, 12:44 PM.

Event Timeline

gberry created this revision.Jul 3 2017, 12:44 PM
hfinkel edited edge metadata.Jul 3 2017, 4:22 PM

Given that I understand the use case here to be the desire to translate metadata on loads into something at the MI level:

Did you consider just adding the metadata operands as MI-level metadata operands to the load? I believe that MI-level metadata operands are only currently used for DBG_VALUEs, but I can imagine using them for more-general information passing. I question came up just the other day (again) about preserving the !unpredictable metadata on branches for MI-level transformations, and so I'm wondering whether it is worth exploring a solution that can handle more than memory accesses.

Alternatively, MMOs currently hold a bunch of pointers to various kinds of Metadata. Should we extend this in some way? One option, for example, would be to allow targets to subclass MachinePointerInfo (and, so, we'd always construct and free these via some target callback), thus allowing the target to add whatever it wants.

All that having been said, AArch64 already uses target MMO flags (for suppressLdStPair), and I'm completely in favor of adding some way to get these in and out of MIR. It would be nice to also let the target callbacks provide names. The syntax could just be to use a quoted string, for example. I believe that's unambiguous.

Given that I understand the use case here to be the desire to translate metadata on loads into something at the MI level:

Did you consider just adding the metadata operands as MI-level metadata operands to the load? I believe that MI-level metadata operands are only currently used for DBG_VALUEs, but I can imagine using them for more-general information passing. I question came up just the other day (again) about preserving the !unpredictable metadata on branches for MI-level transformations, and so I'm wondering whether it is worth exploring a solution that can handle more than memory accesses.

Alternatively, MMOs currently hold a bunch of pointers to various kinds of Metadata. Should we extend this in some way? One option, for example, would be to allow targets to subclass MachinePointerInfo (and, so, we'd always construct and free these via some target callback), thus allowing the target to add whatever it wants.

All that having been said, AArch64 already uses target MMO flags (for suppressLdStPair), and I'm completely in favor of adding some way to get these in and out of MIR. It would be nice to also let the target callbacks provide names. The syntax could just be to use a quoted string, for example. I believe that's unambiguous.

Since I only needed one bit on some loads, the target MMO flags seemed like a perfect fit. I did consider adding target hooks for printing/parsing these flag values, but it didn't seem worth the effort, since I think it will make parsing these particularly complex (e.g. we would need custom token types per target etc.). If we made them quoted strings, that might make it simple enough to be worth while.

Given that I understand the use case here to be the desire to translate metadata on loads into something at the MI level:

Did you consider just adding the metadata operands as MI-level metadata operands to the load? I believe that MI-level metadata operands are only currently used for DBG_VALUEs, but I can imagine using them for more-general information passing. I question came up just the other day (again) about preserving the !unpredictable metadata on branches for MI-level transformations, and so I'm wondering whether it is worth exploring a solution that can handle more than memory accesses.

Alternatively, MMOs currently hold a bunch of pointers to various kinds of Metadata. Should we extend this in some way? One option, for example, would be to allow targets to subclass MachinePointerInfo (and, so, we'd always construct and free these via some target callback), thus allowing the target to add whatever it wants.

All that having been said, AArch64 already uses target MMO flags (for suppressLdStPair), and I'm completely in favor of adding some way to get these in and out of MIR. It would be nice to also let the target callbacks provide names. The syntax could just be to use a quoted string, for example. I believe that's unambiguous.

Since I only needed one bit on some loads, the target MMO flags seemed like a perfect fit. I did consider adding target hooks for printing/parsing these flag values, but it didn't seem worth the effort, since I think it will make parsing these particularly complex (e.g. we would need custom token types per target etc.). If we made them quoted strings, that might make it simple enough to be worth while.

I'd prefer that they be quoted strings; otherwise, I'm happy with the direction.

MatzeB edited edge metadata.Jul 6 2017, 8:34 PM

There is already precedent with
TargetInstrInfo::getSerializableTargetIndices(), getSerializableDirectMachineOperandTargetFlags() and getSerializableBitmaskMachineOperandTargetFlags()

Maybe you can add something for MMO flags in a similar way?

gberry updated this revision to Diff 106295.Jul 12 2017, 1:29 PM

Update to add target hook to print/parse target MMO flags as string literals in MIR text

hfinkel accepted this revision.Jul 12 2017, 1:48 PM

This LGTM.

The getMMOFlags addition can be a separate patch, right? If so, please separate it.

I'd also really like a comment added to include/llvm/CodeGen/MachineMemOperand.h, around here:

// Reserved for use by target-specific passes.
MOTargetFlag1 = 1u << 6,
MOTargetFlag2 = 1u << 7,
MOTargetFlag3 = 1u << 8,

reminding us that if we add more flags here we also need to update the MIR printing/parsing code.

This revision is now accepted and ready to land.Jul 12 2017, 1:48 PM
This revision was automatically updated to reflect the committed changes.