When defining a dialect in TableGen, the bit usePropertiesForAttributes = 1; field controls whether or not to generate a Properties struct for each operation that holds (along with any non-attribute properties) the inherent attributes of that operation. If this flag is set to 0, then inherent and discardable attributes are mixed together into one dictionary.
Supporting this fallback path adds a bunch of extra branches to mlir-tblgen which are not exercised by any upstream code
I propose that, in the near future, use of usPropertiesForAttributes = 0 cause the operation definition tablegen to print a warning about the intended removal.
After said warning has been in at least one LLVM release, the usePropertiesForAttributes field will be removed and the code paths for handling its non-default value will be deleted.
Requested comments
Does anyone know of a reason not to do this, or have a downstream project that’s relying on this setting?
@River707 Thanks for flagging a spot where there’s a usage of getAttrDictionary() that we’ll want to investigate. That could probably be some sort of populateInherentAttrs() usage … or maybe we want new methods, maybe implementing sub-element replacement on property structs.
However, getAttrDictionary() and setAttrs() all already merge together / pull apart inherent and discardable attributes (last I checked), which means that this deprecation is completely independent of that code.
(You’ll note that all of upstream has usePropertiesForAttributes = 1 and that replacement code works correctly.)
I know things still work today without native support, but there were some issues (maybe just compile time?) That prevented some people i know from using properties for attribute storage.
What do you mean? Like testing for the case where usePropertiesForAttributes = 0?
It is true that we don’t have much test coverage left for usePropertiesForAttributes = 0 since we transition, this should actually support even more this RFC marking it as officially deprecated and slotted for removal.
We should definitely work on this: the best way I believe it to slowly work toward deprecating the APIs that are still present for backward compatibility (getAttr(), setAttr(), getAttrDictionary(), …).
Only when we can remove all uses in-tree for these I would have confidence that we don’t have edge cases (using these APIs is functional but “slow” indeed).
If you have some more details on that, I’d appreciate it
More strongly, we have no test coverage for that case.
Yeah, the infrastructure for removing uses of those isn’t quite there yet. I’ve got plans, but … time and bandwidth. I’ll try and put up a “What’s cancelling the propocalypse?” document next week-ish
If the usage of the subelement interface remains functionally correct and as slow as it used to be, I don’t think it should be blocking progress. Regressing its performance significantly would be a blocker, not improving it shouldn’t be.