Repository navigation
Conversation
MTypeTree and MTypeResult are the counterparts of the TypeQuery structure in the LLVM implementation. In MLIR, we already know all types when building the tree, rather than relying on dynamically leverage nodes at runtime as LLVM does. Therefore, the tree here is only used to insert concrete type. To preserve protability, we stil use a tree-like structure and provide query, operator[], and isnert in our MType* classes. However, they never actually build the tree by relying on a string-based parse() function. Instead, the underlying dialect should provide the type information and build the tree. For query, we build the tree instead of using a cache version. This should be fine, as the tree currently has only three levels and should not be too large. For operator[], we follow the same logic as the LLVM implementation, with slight modifications to make the code more readable.
|
Of course, there is no in-tree consumer for this now. I am cleanup my CIR adopter now and will send the patch for the consumer in here. I still try to upstream to here to prevent huge work when rebasing. NOTE: I spend lots of time on separating patch to CIR and unrelated patch. :). |
|
See: Ringo-Leo/Enzyme-CIR@8daff03 for CIR changes |
Antipath1
left a comment
Antipath1
left a comment
There was a problem hiding this comment.
Except for my comments, this does look good to me, although it is a bit unfortunate that we can not really have tests for the TypeTree here, but I get why it is not really possible, since nothing uses the tree upstream yet, and doing that for the LLVM dialect would be far more work than it is worth, I believe.
Antipath1
left a comment
Antipath1
left a comment
There was a problem hiding this comment.
LGTM thank youuuuuuuuu
65e9ab6 to
8841f33
Compare
| class MTypeResults { | ||
| enum class MBaseType { Integer, Float, Pointer, Anything, Unknown }; | ||
|
|
||
| class MConcreteType { |
There was a problem hiding this comment.
can we potentially template the llvm concrete type to avoid redundant code?
aokblast
changed the title
| using namespace mlir; | ||
| using namespace mlir::enzyme; | ||
|
|
||
| void MTypeTree::insert(const std::vector<int> &seq, MConcreteType ct) { |
There was a problem hiding this comment.
can we not just use a [templatized] llvm type tree, do we need this?
There was a problem hiding this comment.
Sorry. There were to many branches work together. We don't templatized TypeTree. The only templatized are ConcreteType and reuse BaseType. Reflect this on the comment and title.
There was a problem hiding this comment.
correct, I am asking why have a distinct MTypeTree, instead of templating the LLVM TypeTree and re-using that?
There was a problem hiding this comment.
I think it is a little bit strange. Functions like IsFullyDetermined, Only, Data0, Lookup, ShiftIndices, toMD is (now) LLVM only as MLIR's type should be deterministic (at least I don't find any case now that needs dynamically leverage on the tree). And even if we need leverage on tree. The meaning of index may be different in MLIR's view.
There was a problem hiding this comment.
memcpy((void*)dst, (void*)src, 8) has an undefined derivative without knowing the underlying type. If a function took in dst and src as void* this would not be possible to differentiate without type information propagation -- so we will eventually need it
There was a problem hiding this comment.
Oh you are right. Reverse mode need such propagation on type. Templatized this now.
aokblast
changed the title
aokblast
changed the title
|
To be clear I'm not saying we should build the full analysis now, but we should use data structures that don't block us from doing so later (and equally ensure we have as little duplication as possible to make sure bug fixes are ported everywhere) |
aokblast
changed the title
aokblast
changed the title
Template TypeTree over the float type handle, like GenericConcreteType, so MLIR reuses it instead of keeping its own MTypeTree. The underlying consumer should implement the required Trait for using. Also, slightly refactor the operator[].