Value traits support #121
Value traits support #121
Conversation
There was a problem hiding this comment.
Thank you for working on this!
One high-level observation is that this new feature should compose well with the superclass feature. In the example dialect, look at StreamReduceOp: it should be possible to add value_traits in there as well, which are then adopted by StreamAddOp etc. Please add that to the .td files as an example and test case.
On the C++ side, it's probably best to move the traits vector from Operation into OperationBase, from where it can be copied from the superclass in OperationBase::init if there is one.
Also... I may have talked about subclassing LlvmEnumAttributeTrait in C++, but after seeing that in code in practice I have to say that the redundancy between indices and classes is confusing. I think it would be best to stick with a single LlvmEnumAttributeTrait class with an m_index member, where the index uses magic numbers to indicate return value and function attributes, just like how it works in LLVM itself.
lib/TableGen/Operations.cpp
Outdated
|
|
||
| const RecordVal *insVal = record->getValue("arguments"); | ||
| std::unordered_map<std::string, unsigned> nameToIndexMap; | ||
| if (const DagInit *DI = dyn_cast<DagInit>(insVal->getValue())){ |
There was a problem hiding this comment.
It is an error for the arguments field to not be of dag type, and we should err on the side of failing loudly on unexpected data. Otherwise, users may end up specifying their operations incorrectly without noticing it.
Just use getValueAsDag here, as is done elsewhere. And the same applies below to results and analogously to value_traits.
There was a problem hiding this comment.
Agree for the arguments field. Changed it to use getValueAsDag. However, results might be empty when working with OpClasses. For the value_traits, I added the fatal_error_report in case there is something other than dag inside of value_traits list.
There was a problem hiding this comment.
For the results case, you can still assume that results is a dag. That should even be ensured by the type system of the TableGen frontend. The only check you gneed is for getNumArgs.
There was a problem hiding this comment.
Sorry, I should've expressed myself better. It would be correct to assume results is a dag, but only when there is a results field. In case of OpClass there is no results field. When field is not present, getValueAsDag returns fatal error. Seems to me things should stay as is for the results case.
sstipano
force-pushed
the
value_traits
branch
2 times, most recently
from
a191e5c to
f8cb9b8
Compare
|
I was working with a bit older llvm commit in my local checkout and I missed the switch to the new Captures attribute. What do you think? |
Co-authored-by: Valentin Churavy <[email protected]>
2 participants
No description provided.