This is an archive of the discontinued LLVM Phabricator instance.

[IR] Merge metadata manipulation code into Value
ClosedPublic

Authored by sepavloff on Sep 16 2019, 9:29 AM.

Details

Summary

Now there are two main classes in Value hierarchy, which support metadata,
these are Instruction and GlobalObject. They implement different APIs for
metadata manipulation, which however overlap. This change moves metadata
manipulation code into Value, so descendant classes can use this code for
their operations on metadata.

No functional changes intended.

Event Timeline

sepavloff created this revision.Sep 16 2019, 9:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 16 2019, 9:29 AM
Herald added a subscriber: hiraditya. · View Herald Transcript

I don't think it should be possible to attach metadata to constants owned by the LLVMContext (such as i32 0) since that seems like a bug, but IIUC this patch is enabling that. Is that intentional? If so, why? If not, can you find a way to avoid making that possible?

sepavloff updated this revision to Diff 220513.Sep 17 2019, 8:36 AM

Restrict types allowed to have metadata

dexonsmith requested changes to this revision.Oct 17 2020, 5:26 AM

Sadly I completely missed your update here. I’m skeptical there’s another Value subclass that needs or should have metadata (do you have something in mind?), but this is nevertheless a massive simplification and I like the direction!

If you’re still interested in pursuing this, please change the members you added to Value not to be public; you can make them protected and then in Instruction and GlobalObject I suggest adding using declarations. If a third category arises we can do the same there.

This revision now requires changes to proceed.Oct 17 2020, 5:26 AM

Made metadata manipulation methods protected

sepavloff edited the summary of this revision. (Show Details)Oct 21 2020, 11:10 PM

Sadly I completely missed your update here. I’m skeptical there’s another Value subclass that needs or should have metadata (do you have something in mind?), but this is nevertheless a massive simplification and I like the direction!

Thank you for feedback!

This change was created to support attributes of basic blocks. They would mark blocks were specific floating point options are in effect and prevent floating point operations from moving between blocks with different FP properties. This idea was not supported by the community, so I agree, no new Value need metadata support. If however this patch could made the code a little better, it would be nice.

If you’re still interested in pursuing this, please change the members you added to Value not to be public; you can make them protected and then in Instruction and GlobalObject I suggest adding using declarations. If a third category arises we can do the same there.

Done.

dexonsmith accepted this revision.Oct 22 2020, 9:38 AM

LGTM. Thanks for your patience with this.

This revision is now accepted and ready to land.Oct 22 2020, 9:38 AM
This revision was landed with ongoing or failed builds.Oct 22 2020, 9:10 PM
This revision was automatically updated to reflect the committed changes.