-
Notifications
You must be signed in to change notification settings - Fork 14.3k
[llvm:ir] Add support for constant data exceeding 4GiB #126481
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -1581,7 +1581,7 @@ static void printConstant(const Constant *COp, unsigned BitWidth, | |||||
bool IsInteger = EltTy->isIntegerTy(); | ||||||
bool IsFP = EltTy->isHalfTy() || EltTy->isFloatTy() || EltTy->isDoubleTy(); | ||||||
unsigned EltBits = EltTy->getPrimitiveSizeInBits(); | ||||||
unsigned E = std::min(BitWidth / EltBits, CDS->getNumElements()); | ||||||
unsigned E = std::min(BitWidth / EltBits, (unsigned)CDS->getNumElements()); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We tested, actually it work well on our llvm-15 based TVM for over half a year... This script print(
'''; ModuleID = 'large_string.c'
source_filename = "large_string.c"
target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-i128:128-f80:128-n8:16:32:64-S128"
target triple = "x86_64-unknown-linux-gnu"
'''
)
print('@.str = private unnamed_addr constant [4294968320 x i8] c"', end='')
for i in range(4294968320//1024):
print('abcdefgh' * 128, end='')
print('", align 1')
print(
'''@s = dso_local local_unnamed_addr global ptr @.str, align 8
!llvm.module.flags = !{!0, !1, !2, !3}
!llvm.ident = !{!4}
!0 = !{i32 1, !"wchar_size", i32 4}
!1 = !{i32 8, !"PIC Level", i32 2}
!2 = !{i32 7, !"PIE Level", i32 2}
!3 = !{i32 7, !"uwtable", i32 2}
!4 = !{!"clang version 21.0.0git (https://github.com/llvm/llvm-project.git 0395fd9680a348c6afc52a165453222c02a3cd48)"}
''') python3 gen_large_string.py > large_string.ll
llc large_string.ll There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll give you that printConstant isn't really important. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "abcdefgh" is not a good string to use because its length is a power of 2; that means you won't notice if integer indexes wrap. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you very much for your meticulous review. #!/bin/bash
cat << EOF
target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-i128:128-f80:128-n8:16:32:64-S128"
target triple = "x86_64-unknown-linux-gnu"
EOF
# generate @S = constant [4294967303 x i8] c"......", align 1
for (( i = 0; i<128; i++)); do
S_1K+='abcdefgh'
done
for (( i = 0; i<1024; i++)); do
S_1M+=$S_1K
done
echo -n "@S = constant [$((1024 * 1024 * 1024 * 4 + 7)) x i8] c\""
for (( i = 0; i<$((1024 * 4)); i++)); do
echo -n $S_1M
done
echo -n 'last7ch'
echo '", align 1'
cat << EOF
!llvm.module.flags = !{!0, !1, !2, !3}
!0 = !{i32 1, !"wchar_size", i32 4}
!1 = !{i32 8, !"PIC Level", i32 2}
!2 = !{i32 7, !"PIE Level", i32 2}
!3 = !{i32 7, !"uwtable", i32 2}
EOF bash gen_large_string.sh > a.ll
llc a.ll There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not just switch it to uint64_t even if it "works"? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't want to make too many changes. Changing unsigned to uint64_t here will lead to a series of modifications to printConstant, but printConstant doesn't actually handle constants exceeding 4GiB |
||||||
if ((BitWidth % EltBits) == 0) { | ||||||
for (unsigned I = 0; I != E; ++I) { | ||||||
if (I != 0) | ||||||
|
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.
Do you also need to update other APIs? ConstantDataSequential::getElementAsInteger() etc.
Uh oh!
There was an error while loading. Please reload this page.
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.
other APIs do not affect the emission of large constant data, so we do not modify them for now.
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.
It looks like the AsmPrinter, at least, is using getElementAsInteger. Please try some tests that don't hit the
CDS->isString()
optimized path.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.
ok, I update
getElementAsInteger
andgetElementAsFloat
, it seems that these two are used in codegen.I test it with this script's generation
I checked that the result is ok. The last number in result asm is
233
and the size is4294967300
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.
Should I add these test to regression tests? but it is really large
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.
Adding a python script as a regression test isn't necessarily an issue, but I assume it would take an excessively large amount of time/memory to process a 4GB file. We don't really have any place in-tree for a test like that. Leaving it out is probably fine... it's here for reference if we need it in the future.