-
Notifications
You must be signed in to change notification settings - Fork 127
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[CIR][CodeGen][Lowering] Supports arrays with trailing zeros #393
Conversation
✅ With the latest revision this PR passed the C/C++ code formatter. |
2f98faf
to
11b8b05
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks great, my 2 cents in the comments
clang/test/CIR/CodeGen/const-array.c
Outdated
|
||
// CHECK: cir.func {{.*@foo}} | ||
// CHECK: %0 = cir.alloca !cir.array<!s32i x 10>, cir.ptr <!cir.array<!s32i x 10>>, ["a"] {alignment = 16 : i64} | ||
// CHECK: %1 = cir.const(#cir.const_array<[#cir.int<1> : !s32i], 9 zeros> : !cir.array<!s32i x 10>) : !cir.array<!s32i x 10> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this. I like how this simplifies and allows postponing all different initialization heuristics for structs (as you linked for unimplemented features).
Is there any special reason to print "9 zeros" instead of just "trailingzeros"? Are there other uses for the actual value?
I can see this being just implemented as a UnitAttr
, which could optionally print "trailingzeros". Sounds like mlir::cir::ArrayType
and elts
can be used to compute how many trailing zeros there are when lowering?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bcardosolopes
yes, it's better! Though I used just bool
instead of UnitAttr
, hope it's ok)
So let's take a look again!
4e069c6
to
79d4dc7
Compare
@lanza just rebased and this has the side effect of breaking GH PR workflow, making it impossible to review, apologies. Please rebase! |
d423157
to
18f2b76
Compare
@bcardosolopes done! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost there, few more comments!
} | ||
|
||
void ConstArrayAttr::print(::mlir::AsmPrinter &printer) const { | ||
printer << "<"; | ||
printer.printStrippedAttrOrType(getElts()); | ||
if (auto zeros = getTrailingZerosNum()) | ||
printer << ", trailingZeros"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For printed CIR I suggest we use trailing_zeros
or trailingzeros
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
if (auto str = elts.dyn_cast<mlir::StringAttr>()) | ||
return typeSize - str.size(); | ||
else | ||
return typeSize - getElts().cast<mlir::ArrayAttr>().size(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the trailing number of zeros is a constant, can we compute this during attr creation and only return a value here instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point!
@bcardosolopes done! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! LGTM
This PR adds support for constant arrays with trailing zeros. The original `CodeGen` does the following: once a constant array contain trailing zeros, a struct with two members is generated: initialized elements and `zeroinitializer` for the remaining part. And depending on some conditions, `memset` or `memcpy` are emitted. In the latter case a global const array is created. Well, we may go this way, but it requires us to implement [features](https://github.com/llvm/clangir/blob/main/clang/lib/CIR/CodeGen/CIRGenDecl.cpp#L182) that are not implemented yet. Another option is to add one more parameter to the `constArrayAttr` and utilize it during the lowering. So far I chose this way, but if you have any doubts, we can discuss here. So we just emit constant array as usually and once there are trailing zeros, lower this arrray (i.e. an attribute) as a value. I added a couple of tests and will add more, once we agree on the approach. So far I marked the PR as a draft one.
This PR adds support for constant arrays with trailing zeros. The original `CodeGen` does the following: once a constant array contain trailing zeros, a struct with two members is generated: initialized elements and `zeroinitializer` for the remaining part. And depending on some conditions, `memset` or `memcpy` are emitted. In the latter case a global const array is created. Well, we may go this way, but it requires us to implement [features](https://github.com/llvm/clangir/blob/main/clang/lib/CIR/CodeGen/CIRGenDecl.cpp#L182) that are not implemented yet. Another option is to add one more parameter to the `constArrayAttr` and utilize it during the lowering. So far I chose this way, but if you have any doubts, we can discuss here. So we just emit constant array as usually and once there are trailing zeros, lower this arrray (i.e. an attribute) as a value. I added a couple of tests and will add more, once we agree on the approach. So far I marked the PR as a draft one.
This PR adds support for constant arrays with trailing zeros. The original `CodeGen` does the following: once a constant array contain trailing zeros, a struct with two members is generated: initialized elements and `zeroinitializer` for the remaining part. And depending on some conditions, `memset` or `memcpy` are emitted. In the latter case a global const array is created. Well, we may go this way, but it requires us to implement [features](https://github.com/llvm/clangir/blob/main/clang/lib/CIR/CodeGen/CIRGenDecl.cpp#L182) that are not implemented yet. Another option is to add one more parameter to the `constArrayAttr` and utilize it during the lowering. So far I chose this way, but if you have any doubts, we can discuss here. So we just emit constant array as usually and once there are trailing zeros, lower this arrray (i.e. an attribute) as a value. I added a couple of tests and will add more, once we agree on the approach. So far I marked the PR as a draft one.
This PR adds support for constant arrays with trailing zeros. The original `CodeGen` does the following: once a constant array contain trailing zeros, a struct with two members is generated: initialized elements and `zeroinitializer` for the remaining part. And depending on some conditions, `memset` or `memcpy` are emitted. In the latter case a global const array is created. Well, we may go this way, but it requires us to implement [features](https://github.com/llvm/clangir/blob/main/clang/lib/CIR/CodeGen/CIRGenDecl.cpp#L182) that are not implemented yet. Another option is to add one more parameter to the `constArrayAttr` and utilize it during the lowering. So far I chose this way, but if you have any doubts, we can discuss here. So we just emit constant array as usually and once there are trailing zeros, lower this arrray (i.e. an attribute) as a value. I added a couple of tests and will add more, once we agree on the approach. So far I marked the PR as a draft one.
This PR adds support for constant arrays with trailing zeros. The original `CodeGen` does the following: once a constant array contain trailing zeros, a struct with two members is generated: initialized elements and `zeroinitializer` for the remaining part. And depending on some conditions, `memset` or `memcpy` are emitted. In the latter case a global const array is created. Well, we may go this way, but it requires us to implement [features](https://github.com/llvm/clangir/blob/main/clang/lib/CIR/CodeGen/CIRGenDecl.cpp#L182) that are not implemented yet. Another option is to add one more parameter to the `constArrayAttr` and utilize it during the lowering. So far I chose this way, but if you have any doubts, we can discuss here. So we just emit constant array as usually and once there are trailing zeros, lower this arrray (i.e. an attribute) as a value. I added a couple of tests and will add more, once we agree on the approach. So far I marked the PR as a draft one.
This PR adds support for constant arrays with trailing zeros. The original `CodeGen` does the following: once a constant array contain trailing zeros, a struct with two members is generated: initialized elements and `zeroinitializer` for the remaining part. And depending on some conditions, `memset` or `memcpy` are emitted. In the latter case a global const array is created. Well, we may go this way, but it requires us to implement [features](https://github.com/llvm/clangir/blob/main/clang/lib/CIR/CodeGen/CIRGenDecl.cpp#L182) that are not implemented yet. Another option is to add one more parameter to the `constArrayAttr` and utilize it during the lowering. So far I chose this way, but if you have any doubts, we can discuss here. So we just emit constant array as usually and once there are trailing zeros, lower this arrray (i.e. an attribute) as a value. I added a couple of tests and will add more, once we agree on the approach. So far I marked the PR as a draft one.
This PR adds support for constant arrays with trailing zeros. The original `CodeGen` does the following: once a constant array contain trailing zeros, a struct with two members is generated: initialized elements and `zeroinitializer` for the remaining part. And depending on some conditions, `memset` or `memcpy` are emitted. In the latter case a global const array is created. Well, we may go this way, but it requires us to implement [features](https://github.com/llvm/clangir/blob/main/clang/lib/CIR/CodeGen/CIRGenDecl.cpp#L182) that are not implemented yet. Another option is to add one more parameter to the `constArrayAttr` and utilize it during the lowering. So far I chose this way, but if you have any doubts, we can discuss here. So we just emit constant array as usually and once there are trailing zeros, lower this arrray (i.e. an attribute) as a value. I added a couple of tests and will add more, once we agree on the approach. So far I marked the PR as a draft one.
This PR adds support for constant arrays with trailing zeros. The original `CodeGen` does the following: once a constant array contain trailing zeros, a struct with two members is generated: initialized elements and `zeroinitializer` for the remaining part. And depending on some conditions, `memset` or `memcpy` are emitted. In the latter case a global const array is created. Well, we may go this way, but it requires us to implement [features](https://github.com/llvm/clangir/blob/main/clang/lib/CIR/CodeGen/CIRGenDecl.cpp#L182) that are not implemented yet. Another option is to add one more parameter to the `constArrayAttr` and utilize it during the lowering. So far I chose this way, but if you have any doubts, we can discuss here. So we just emit constant array as usually and once there are trailing zeros, lower this arrray (i.e. an attribute) as a value. I added a couple of tests and will add more, once we agree on the approach. So far I marked the PR as a draft one.
This PR adds support for constant arrays with trailing zeros. The original `CodeGen` does the following: once a constant array contain trailing zeros, a struct with two members is generated: initialized elements and `zeroinitializer` for the remaining part. And depending on some conditions, `memset` or `memcpy` are emitted. In the latter case a global const array is created. Well, we may go this way, but it requires us to implement [features](https://github.com/llvm/clangir/blob/main/clang/lib/CIR/CodeGen/CIRGenDecl.cpp#L182) that are not implemented yet. Another option is to add one more parameter to the `constArrayAttr` and utilize it during the lowering. So far I chose this way, but if you have any doubts, we can discuss here. So we just emit constant array as usually and once there are trailing zeros, lower this arrray (i.e. an attribute) as a value. I added a couple of tests and will add more, once we agree on the approach. So far I marked the PR as a draft one.
This PR adds support for constant arrays with trailing zeros. The original `CodeGen` does the following: once a constant array contain trailing zeros, a struct with two members is generated: initialized elements and `zeroinitializer` for the remaining part. And depending on some conditions, `memset` or `memcpy` are emitted. In the latter case a global const array is created. Well, we may go this way, but it requires us to implement [features](https://github.com/llvm/clangir/blob/main/clang/lib/CIR/CodeGen/CIRGenDecl.cpp#L182) that are not implemented yet. Another option is to add one more parameter to the `constArrayAttr` and utilize it during the lowering. So far I chose this way, but if you have any doubts, we can discuss here. So we just emit constant array as usually and once there are trailing zeros, lower this arrray (i.e. an attribute) as a value. I added a couple of tests and will add more, once we agree on the approach. So far I marked the PR as a draft one.
This PR adds support for constant arrays with trailing zeros.
The original
CodeGen
does the following: once a constant array contain trailing zeros, a struct with two members is generated: initialized elements andzeroinitializer
for the remaining part. And depending on some conditions,memset
ormemcpy
are emitted. In the latter case a global const array is created.Well, we may go this way, but it requires us to implement features that are not implemented yet.
Another option is to add one more parameter to the
constArrayAttr
and utilize it during the lowering. So far I chose this way, but if you have any doubts, we can discuss here. So we just emit constant array as usually and once there are trailing zeros, lower this arrray (i.e. an attribute) as a value.I added a couple of tests and will add more, once we agree on the approach. So far I marked the PR as a draft one.