Skip to content
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

Revise usage of const_struct and const_array to enhance codegen parity #181

Open
sitio-couto opened this issue Jul 17, 2023 · 0 comments
Open
Labels
enhancement New feature or request

Comments

@sitio-couto
Copy link
Collaborator

// TODO(cir): constant arrays are currently just pushed into the stack using
// the store instruction, instead of being stored as global variables and
// then memcopyied into the stack (as done in Clang).
else if (auto arrTy = op.getType().dyn_cast<mlir::cir::ArrayType>()) {
// Fetch operation constant array initializer.
auto constArr = op.getValue().dyn_cast<mlir::cir::ConstArrayAttr>();
if (!constArr)
return op.emitError() << "array does not have a constant initializer";
// Lower constant array initializer.
auto denseAttr = lowerConstArrayAttr(constArr, typeConverter);
if (!denseAttr.has_value()) {
op.emitError()
<< "unsupported lowering for #cir.const_array with element type "
<< arrTy.getEltType();
return mlir::failure();
}
attr = denseAttr.value();
} else

Note: This is indeed an interesting difference, if we forget about LLVM and think only about CIR for a moment, we're also not being uniform right now (my fault), given that const_struct attrs are used inline within cir.const and others are going through globals, at some point we need to migrate more.

Originally posted by @bcardosolopes in #171 (review)

I wonder if this approach is actually better than what clang does, given the lack of memcopy/globals being involved - OTOH we might be loosing uniquing from unnamed address tagged globals. Any thoughts @htyu ?

Originally posted by @bcardosolopes in #171 (comment)

An explicit assignment better supports later optimization passes than a memcpy does, which always requires an additional step of analysis. However from performance point of view an explicit assignment would be as less efficient as memcpy does, but I guess we can always push the conversion to memcpy to llvm.

Originally posted by @htyu in #171 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants