Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
test/Transforms/InstCombine/str-int.ll | ||
---|---|---|
6 ↗ | (On Diff #141570) | @strtol_dec? |
10 ↗ | (On Diff #141570) | Needs negative test with endptr not being null. |
11 ↗ | (On Diff #141570) | Do we care about the return type here? |
16 ↗ | (On Diff #141570) | I'd like to see one test with base = 0. |
18 ↗ | (On Diff #141570) | I'm not sure these ; Function Attrs comments are needed. |
19 ↗ | (On Diff #141570) | @strtol_hex? |
lib/Transforms/Utils/SimplifyLibCalls.cpp | ||
---|---|---|
1693 ↗ | (On Diff #141684) |
I'm guessing you want to check the third operand here, not the second one. |
lib/Transforms/Utils/SimplifyLibCalls.cpp | ||
---|---|---|
1697 ↗ | (On Diff #141686) | Oh, CI is CallInst, not ConstantInt, never mind then. |
test/Transforms/InstCombine/str-int.ll | ||
5 ↗ | (On Diff #141686) | The STRTOL(3) says that strtol returns long int, so it would still be good for the tests to represent that. |
lib/Transforms/Utils/SimplifyLibCalls.cpp | ||
---|---|---|
1683 ↗ | (On Diff #141694) | Both of these getConstantStringInfo() == false are also not covered with tests, i think? |
1696 ↗ | (On Diff #141694) | You also probably want a negative test with this third operand being not a constant (an argument of the test function) |
test/Transforms/InstCombine/str-int-2.ll | ||
4 ↗ | (On Diff #141694) | But this is just 1<<31, it fits into i32 as-is. |
6 ↗ | (On Diff #141694) | I'm not sure why you need to separate it into two test files? |
lib/Transforms/Utils/SimplifyLibCalls.cpp | ||
---|---|---|
1685 ↗ | (On Diff #141712) | I'm worried about host behavior leaking through here; specifically, the "undefined" values for overflow might vary. Could you use some function with more predictable behavior here? (Maybe something based on llvm::consumeSignedInteger()?) |
1697 ↗ | (On Diff #141686) | This is missing some checks. In particular, what happens if the host's "long" is a different width from the target's "long"? What happens if strtol sets errno? What happens if the target program is using a locale where isspace() doesn't match the C locale's isspace()? |
lib/Transforms/Utils/SimplifyLibCalls.cpp | ||
---|---|---|
1697 ↗ | (On Diff #141686) | I dont know about that thing with locale. I dont think we have to care about this weirdness. |
lib/Transforms/Utils/SimplifyLibCalls.cpp | ||
---|---|---|
1697 ↗ | (On Diff #141686) | isspace() in all locales should return true for all the characters where it returns true in the C locale; it's just a question of whether there are some additional characters which also count as spaces. So I think you'll get the right behavior if you refuse to fold in cases where strtol would fail in the C locale. |
Is patch now ok for you, @efriedma ?
lib/Transforms/Utils/SimplifyLibCalls.cpp | ||
---|---|---|
1685 ↗ | (On Diff #141712) | We should call that function when we can call strtol directly? |
1697 ↗ | (On Diff #141686) | Errno can be checked, can be fixed in this patch. But why we should check target host vs host long ? |
1697 ↗ | (On Diff #141686) | Updated patch. Check for "C" locale added. |
lib/Transforms/Utils/SimplifyLibCalls.cpp | ||
---|---|---|
1697 ↗ | (On Diff #141686) | The result of strtol("2147483648", 0, 0) varies depending on whether long is 32 bits or 64 bits. |
lib/Transforms/Utils/SimplifyLibCalls.cpp | ||
---|---|---|
1685 ↗ | (On Diff #141712) | I guess we can just call strtol, sure. |
lib/Transforms/Utils/SimplifyLibCalls.cpp | ||
---|---|---|
1685 ↗ | (On Diff #141712) | So now is this patch good to go? |
lib/Transforms/Utils/SimplifyLibCalls.cpp | ||
---|---|---|
1716 ↗ | (On Diff #142087) | DL.getTypeAllocSize(CI->getType()) != sizeof(long int) will cause your testcase to fail on 32-bit hosts (because we'll refuse the fold the call). Can you use strtoll plus a range check, instead? |
1685 ↗ | (On Diff #141712) | You didn't actually change the code to call strtol instead of atoi. |
lib/Transforms/Utils/SimplifyLibCalls.cpp | ||
---|---|---|
1716 ↗ | (On Diff #142087) | But problem is .. errs() << std::numeric_limits<long int>::max() << "\n"; 9223372036854775807 So we need to rely on errno, I think |
lib/Transforms/Utils/SimplifyLibCalls.cpp | ||
---|---|---|
1716 ↗ | (On Diff #142087) | By "range check", I mean in addition to the errno check, if CI->getType() is a 32-bit integer, check that the result of strtoll fits into a 32-bit integer. |
lib/Transforms/Utils/SimplifyLibCalls.cpp | ||
---|---|---|
1716 ↗ | (On Diff #142087) | Oh :D ok. Will fix. |
lib/Transforms/Utils/SimplifyLibCalls.cpp | ||
---|---|---|
1716 ↗ | (On Diff #142087) | The range check is still wrong. You can try building 32-bit LLVM yourself by setting the CMake option LLVM_BUILD_32_BITS. |
lib/Transforms/Utils/SimplifyLibCalls.cpp | ||
---|---|---|
1716 ↗ | (On Diff #142087) | So how to fix it? |
lib/Transforms/Utils/SimplifyLibCalls.cpp | ||
---|---|---|
1716 ↗ | (On Diff #142087) | I have no idea what I should do to fix it... :/ |
lib/Transforms/Utils/SimplifyLibCalls.cpp | ||
---|---|---|
1716 ↗ | (On Diff #142087) | I think the most readable solution is something like int Bits = CI->getType()->getPrimitiveSizeInBits(); if (Result < llvm::minIntN(Bits) || Result > llvm::maxIntN(Bits)) return nullptr;. |
lib/Transforms/Utils/SimplifyLibCalls.cpp | ||
---|---|---|
1716 ↗ | (On Diff #142087) | Actually, you can simplify that to just if (!isIntN(CI->getType()->getPrimitiveSizeInBits(), Result)) return nullptr;. |
lib/Transforms/Utils/SimplifyLibCalls.cpp | ||
---|---|---|
1716 ↗ | (On Diff #142087) | Thank you so much! (I have no idea about that API) |
@efriedma , is now everything ok?
I would send some more patches for SimplifyLibCalls where I would like to use newly introduced isCLocale function.
I'd like to see a second testcase which checks our transforms on declare i32 @strtol(i8*, i8**, i32) (which is the signature on 32-bit targets).
lib/Transforms/Utils/SimplifyLibCalls.cpp | ||
---|---|---|
108 ↗ | (On Diff #142568) | We can safely assume nobody is messing with the locale in the compiler itself. The problem comes when someone uses setlocale on the target (in the compiled program); that's not something you can detect in general. Fortunately, we can still handle the interesting cases, I think: if we assume all possible target locales are ASCII supersets, or something close (which we already assume in other places), then if strtoll successfully parses a number on the host (i.e. nptr != endptr), it will also successfully parse the same way on the target. If the string starts with a non-ASCII character, we probably have to assume it could be a space in some locale. |
1702 ↗ | (On Diff #142568) | Can you just return Result here, instead of calling atoi? |
1720 ↗ | (On Diff #142568) | strtoll can also fail with EINVAL, if the base is invalid. But the exact behavior isn't specified in the C standard, so probably better to check the base ourselves, explicitly. |
2329 ↗ | (On Diff #142568) | Can you also add cases here for atol, atoll, and strtoll? (Your current implementations should just work.) |
lib/Transforms/Utils/SimplifyLibCalls.cpp | ||
---|---|---|
2329 ↗ | (On Diff #142568) | Maybe one test each to make sure we recognize the names, and a test to make sure 64-bit atoll actually allows inputs greater than 2**32; we don't need all the tests for each one given the code is the same. |
Added tests for atol, atoll, strtoll.
lib/Transforms/Utils/SimplifyLibCalls.cpp | ||
---|---|---|
1723 ↗ | (On Diff #142680) | :) Ahh |
Can you share some of the code between optimizeAtoi and optimizeStrtol?
lib/Transforms/Utils/SimplifyLibCalls.cpp | ||
---|---|---|
1719 ↗ | (On Diff #142684) | Like I mentioned before, we don't need this check; it's safe to assume the compiler is running in the C locale. |
1737 ↗ | (On Diff #142684) | Needs a comment here to explain why you're checking this. |
It's better to ask someone who looked at this patch to commit it; I didn't follow this review this at all.
Ok, I was not sure if @efriedma can merge it since I see you merge patches every day in the "InstCombine" part. Sorry then :)
I was going to commit this, but then I discovered that str-int.ll and str-int-2.ll are identical. Did you accidentally overwrite your changes?