Skip to content

Conversation

@ChuanqiXu9
Copy link
Member

Close #975

See the attached test case for example

if (auto attr = mlir::dyn_cast<mlir::StringAttr>(constArr.getElts())) {
init = rewriter.getStringAttr(attr.getValue());
llvm::SmallString<256> literal(attr.getValue());
if (constArr.getTrailingZerosNum())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. For regular strings we just emit the string directly (https://godbolt.org/z/a6xWGx3hf), but for trailing NUL bytes we use trailing zeros (https://godbolt.org/z/r54vvKPb7). Maybe we shouldn't be generating the trailing zeroes instead (assuming there's no technical limitation around that)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LLVM will generate the trailing zero: https://godbolt.org/z/4G75e4P1o . I feel better if we can get align with LLVM in the traditional pipeline. (this is LowerToLLVM.cpp and it shouldn't affect how we generate cir)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps OG should be fixed too? Anyways, in the meantime I'm sold on keeping it consistent with OG

if (auto attr = mlir::dyn_cast<mlir::StringAttr>(constArr.getElts())) {
init = rewriter.getStringAttr(attr.getValue());
llvm::SmallString<256> literal(attr.getValue());
if (constArr.getTrailingZerosNum())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps OG should be fixed too? Anyways, in the meantime I'm sold on keeping it consistent with OG

@bcardosolopes bcardosolopes merged commit e505aae into llvm:main Oct 16, 2024
7 checks passed
keryell pushed a commit to keryell/clangir that referenced this pull request Oct 19, 2024
lanza pushed a commit that referenced this pull request Nov 5, 2024
lanza pushed a commit that referenced this pull request Mar 18, 2025
terapines-osc-cir pushed a commit to Terapines/clangir that referenced this pull request Sep 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Failed to emit string literals ends with '\0'

3 participants