[SimplifyLibcalls] Atoi, strtol replacements

[lebedev.ri]

test/Transforms/InstCombine/str-int.ll

7

@strtol_dec?

11

Needs negative test with endptr not being null.

12

Do we care about the return type here?
I'd think you can simplify the tests and drop trunc.

17

I'd like to see one test with base = 0.

19

I'm not sure these ; Function Attrs comments are needed.

20

@strtol_hex?

[lebedev.ri]

lib/Transforms/Utils/SimplifyLibCalls.cpp

1693

1. elide {}
2. this is clearly broken. you check the same operand here and in the next if().

I'm guessing you want to check the third operand here, not the second one.
And do add the test.

[lebedev.ri]

lib/Transforms/Utils/SimplifyLibCalls.cpp

1697

base is i32, but strtol returns long int (i64?), so i don't think using CI->getType() is correct.

test/Transforms/InstCombine/str-int.ll

6

So i suppose this needs a test with larger-than-32bit-integer.

[lebedev.ri]

lib/Transforms/Utils/SimplifyLibCalls.cpp

1697

Oh, CI is CallInst, not ConstantInt, never mind then.

test/Transforms/InstCombine/str-int.ll

6

The STRTOL(3) says that strtol returns long int, so it would still be good for the tests to represent that.
(i.e. please change this strtol to return i64, and add a test from ^ that comment.)

[lebedev.ri]

lib/Transforms/Utils/SimplifyLibCalls.cpp

1683

Both of these getConstantStringInfo() == false are also not covered with tests, i think?

1696

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.
Maybe 4294967296 (1<<32) ?

6 ↗

(On Diff #141694)

I'm not sure why you need to separate it into two test files?
Couldn't strtol in str-int.ll simply return i64?

[efriedma]

lib/Transforms/Utils/SimplifyLibCalls.cpp

1685

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

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()?

[xbolva00]

Abandoned.

lib/Transforms/Utils/SimplifyLibCalls.cpp

1697

Ah yes. So I will have to close this patch since we cant optimize this calls.

test/Transforms/InstCombine/str-int-2.ll

4 ↗

(On Diff #141694)

Updated.

[xbolva00]

lib/Transforms/Utils/SimplifyLibCalls.cpp

1697

I dont know about that thing with locale. I dont think we have to care about this weirdness.

[xbolva00]

Errno fix

[efriedma]

lib/Transforms/Utils/SimplifyLibCalls.cpp

1697

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.

[xbolva00]

Added locale check for atoi

[xbolva00]

Is patch now ok for you, @efriedma ?

lib/Transforms/Utils/SimplifyLibCalls.cpp

1685

We should call that function when we can call strtol directly?

1697

Errno can be checked, can be fixed in this patch.

But why we should check target host vs host long ?

1697

Updated patch. Check for "C" locale added.

[efriedma]

lib/Transforms/Utils/SimplifyLibCalls.cpp

1697

The result of strtol("2147483648", 0, 0) varies depending on whether long is 32 bits or 64 bits.

[xbolva00]

Fixed things noted by @efriedma

[efriedma]

lib/Transforms/Utils/SimplifyLibCalls.cpp

1685

I guess we can just call strtol, sure.

[xbolva00]

lib/Transforms/Utils/SimplifyLibCalls.cpp

1685

So now is this patch good to go?

[efriedma]

lib/Transforms/Utils/SimplifyLibCalls.cpp

1685

You didn't actually change the code to call strtol instead of atoi.

1710

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?

[xbolva00]

lib/Transforms/Utils/SimplifyLibCalls.cpp

1710

But problem is ..

errs() << std::numeric_limits<long int>::max() << "\n";
errs() << std::numeric_limits<long long int>::max() << "\n";

9223372036854775807
9223372036854775807

So we need to rely on errno, I think

[efriedma]

lib/Transforms/Utils/SimplifyLibCalls.cpp

1710

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.

[xbolva00]

Updated, use strtoll now. @efriedma

[xbolva00]

lib/Transforms/Utils/SimplifyLibCalls.cpp

1710

Oh :D ok. Will fix.

[efriedma]

lib/Transforms/Utils/SimplifyLibCalls.cpp

1710

The range check is still wrong. You can try building 32-bit LLVM yourself by setting the CMake option LLVM_BUILD_32_BITS.

[xbolva00]

lib/Transforms/Utils/SimplifyLibCalls.cpp

1710

So how to fix it?

[xbolva00]

lib/Transforms/Utils/SimplifyLibCalls.cpp

1710

I have no idea what I should do to fix it... :/

[efriedma]

lib/Transforms/Utils/SimplifyLibCalls.cpp

1710

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;.

[efriedma]

lib/Transforms/Utils/SimplifyLibCalls.cpp

1710

Actually, you can simplify that to just if (!isIntN(CI->getType()->getPrimitiveSizeInBits(), Result)) return nullptr;.

[xbolva00]

lib/Transforms/Utils/SimplifyLibCalls.cpp

1710

Thank you so much! (I have no idea about that API)

[xbolva00]

@efriedma , is now everything ok?

I would send some more patches for SimplifyLibCalls where I would like to use newly introduced isCLocale function.

[efriedma]

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

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.

1696

Can you just return Result here, instead of calling atoi?

1714

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.

2300

Can you also add cases here for atol, atoll, and strtoll? (Your current implementations should just work.)

[xbolva00]

Updated with changes requested by @efriedma .

[xbolva00]

lib/Transforms/Utils/SimplifyLibCalls.cpp

1714

So I will add check for base if zero or between 2 and 36.

2300

Ok, I can add it.

Current tests in this patch are good enough or then should be new ones for atol, atoll, and strtoll too?

[efriedma]

lib/Transforms/Utils/SimplifyLibCalls.cpp

1717

Where did 32 come from?

1728

This is a use-after-free (Str.str() allocates a temporary std::string).

[efriedma]

lib/Transforms/Utils/SimplifyLibCalls.cpp

2300

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.

[xbolva00]

Updated, fixed things.

[xbolva00]

Added tests for atol, atoll, strtoll.

lib/Transforms/Utils/SimplifyLibCalls.cpp

1717

:) Ahh

[xbolva00]

ping

[efriedma]

Can you share some of the code between optimizeAtoi and optimizeStrtol?

lib/Transforms/Utils/SimplifyLibCalls.cpp

1713

Like I mentioned before, we don't need this check; it's safe to assume the compiler is running in the C locale.

1731

Needs a comment here to explain why you're checking this.

[xbolva00]

Adressed notes by @efriedma

[efriedma]

LGTM

[xbolva00]

Now you can merge this patch, @spatel

[spatel]

In D45418#1072571, @xbolva00 wrote:

Now you can merge this patch, @spatel

It's better to ask someone who looked at this patch to commit it; I didn't follow this review this at all.

[xbolva00]

In D45418#1072729, @spatel wrote:

In D45418#1072571, @xbolva00 wrote:

Now you can merge this patch, @spatel

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 :)

[efriedma]

I'll merge it tomorrow.

[efriedma]

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?

[xbolva00]

In D45418#1074260, @efriedma wrote:

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?

Updated.

[xbolva00]

Ping (merge request)

[xbolva00]

This comment was removed by xbolva00.

include/llvm/Transforms/Utils/SimplifyLibCalls.h

private:
 
   // Integer Library Call Optimizations
   Value *optimizeFFS(CallInst *CI, IRBuilder<> &B);
   Value *optimizeFls(CallInst *CI, IRBuilder<> &B);
   Value *optimizeAbs(CallInst *CI, IRBuilder<> &B);
   Value *optimizeIsDigit(CallInst *CI, IRBuilder<> &B);
   Value *optimizeIsAscii(CallInst *CI, IRBuilder<> &B);
   Value *optimizeToAscii(CallInst *CI, IRBuilder<> &B);
+  Value *optimizeAtoi(CallInst *CI, IRBuilder<> &B);
+  Value *optimizeStrtol(CallInst *CI, IRBuilder<> &B);
 
   // Formatting and IO Library Call Optimizations
   Value *optimizeErrorReporting(CallInst *CI, IRBuilder<> &B,
                                 int StreamArg = -1);
   Value *optimizePrintF(CallInst *CI, IRBuilder<> &B);
   Value *optimizeSPrintF(CallInst *CI, IRBuilder<> &B);
   Value *optimizeFPrintF(CallInst *CI, IRBuilder<> &B);
   Value *optimizeFWrite(CallInst *CI, IRBuilder<> &B);

lib/Transforms/Utils/SimplifyLibCalls.cpp

static bool callHasFloatingPointArgument(const CallInst *CI) {
   return any_of(CI->operands(), [](const Use &OI) {
     return OI->getType()->isFloatingPointTy();
   });
 }
 
 //===----------------------------------------------------------------------===//
 // String and Memory Library Call Optimizations
 //===----------------------------------------------------------------------===//
 
 Value *LibCallSimplifier::optimizeStrCat(CallInst *CI, IRBuilder<> &B) {
   // Extract some information from the instruction
   Value *Dst = CI->getArgOperand(0);
   Value *Src = CI->getArgOperand(1);
 
   // See if we can get the length of the input string.
   uint64_t Len = GetStringLength(Src);
   if (Len == 0)

 }
 
 Value *LibCallSimplifier::optimizeToAscii(CallInst *CI, IRBuilder<> &B) {
   // toascii(c) -> c & 0x7f
   return B.CreateAnd(CI->getArgOperand(0),
                      ConstantInt::get(CI->getType(), 0x7F));
 }
 
+Value *LibCallSimplifier::optimizeAtoi(CallInst *CI, IRBuilder<> &B) {
+  StringRef Str;
+  if (!getConstantStringInfo(CI->getArgOperand(0), Str))
+    return nullptr;
+
+  return ConstantInt::get(CI->getType(), atoi(Str.str().c_str()));
+}
+
+Value *LibCallSimplifier::optimizeStrtol(CallInst *CI, IRBuilder<> &B) {
+  StringRef Str;
+  if (!getConstantStringInfo(CI->getArgOperand(0), Str))
+    return nullptr;
+
+  if (!isa<ConstantPointerNull>(CI->getArgOperand(1)))
+    return nullptr;
+
+  if (ConstantInt *CInt = dyn_cast<ConstantInt>(CI->getArgOperand(2)))
+    return ConstantInt::get(CI->getType(), strtol(Str.str().c_str(), nullptr,
+                                                  CInt->getSExtValue()));
+
+  return nullptr;
+}
+
 //===----------------------------------------------------------------------===//
 // Formatting and IO Library Call Optimizations
 //===----------------------------------------------------------------------===//
 
 static bool isReportingError(Function *Callee, CallInst *CI, int StreamArg);
 
 Value *LibCallSimplifier::optimizeErrorReporting(CallInst *CI, IRBuilder<> &B,
                                                  int StreamArg) {
   Function *Callee = CI->getCalledFunction();
   // Error reporting calls should be cold, mark them as such.
   // This applies even to non-builtin calls: it is only a hint and applies to
   // functions that the frontend might not understand as builtins.
 
   // This heuristic was suggested in:
   // Improving Static Branch Prediction in a Compiler
   // Brian L. Deitrich, Ben-Chung Cheng, Wen-mei W. Hwu
   // Proceedings of PACT'98, Oct. 1998, IEEE
   if (!CI->hasFnAttr(Attribute::Cold) &&
       isReportingError(Callee, CI, StreamArg)) {
     CI->addAttribute(AttributeList::FunctionIndex, Attribute::Cold);
   }
 
   return nullptr;
 }
 
 static bool isReportingError(Function *Callee, CallInst *CI, int StreamArg) {
   if (!Callee || !Callee->isDeclaration())
     return false;
 
   if (StreamArg < 0)
     return true;
 
   // These functions might be considered cold, but only if their stream
   // argument is stderr.
 
   if (StreamArg >= (int)CI->getNumArgOperands())
     return false;
if (TLI->getLibFunc(*Callee, Func) && TLI->has(Func)) {
     case LibFunc_llabs:
       return optimizeAbs(CI, Builder);
     case LibFunc_isdigit:
       return optimizeIsDigit(CI, Builder);
     case LibFunc_isascii:
       return optimizeIsAscii(CI, Builder);
     case LibFunc_toascii:
       return optimizeToAscii(CI, Builder);
+    case LibFunc_atoi:
+      return optimizeAtoi(CI, Builder);
+    case LibFunc_strtol:
+      return optimizeStrtol(CI, Builder);
     case LibFunc_printf:
       return optimizePrintF(CI, Builder);
     case LibFunc_sprintf:
       return optimizeSPrintF(CI, Builder);
     case LibFunc_fprintf:
       return optimizeFPrintF(CI, Builder);
     case LibFunc_fwrite:
       return optimizeFWrite(CI, Builder);

test/Transforms/InstCombine/str-int.ll

+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt < %s -instcombine -S | FileCheck %s
+
+@.str = private unnamed_addr constant [3 x i8] c"12\00", align 1
+@.str.1 = private unnamed_addr constant [2 x i8] c"0\00", align 1
+@.str.2 = private unnamed_addr constant [11 x i8] c"4294967296\00", align 1
+
+declare i64 @strtol(i8*, i8**, i32) #1
+declare i32 @atoi(i8*) #2
+
+define i64 @strtol_dec() #0 {
+; CHECK-LABEL: @strtol_dec(
+; CHECK-NEXT:    ret i64 12
+;
+  %call = call i64 @strtol(i8* getelementptr inbounds ([3 x i8], [3 x i8]* @.str, i32 0, i32 0), i8** null, i32 10) #2
+  ret i64 %call
+}
+
+define i64 @strtol_base_zero() #0 {
+; CHECK-LABEL: @strtol_base_zero(
+; CHECK-NEXT:    ret i64 12
+;
+  %call = call i64 @strtol(i8* getelementptr inbounds ([3 x i8], [3 x i8]* @.str, i32 0, i32 0), i8** null, i32 0) #2
+  ret i64 %call
+}
+
+define i64 @strtol_hex() #0 {
+; CHECK-LABEL: @strtol_hex(
+; CHECK-NEXT:    ret i64 18
+;
+  %call = call i64 @strtol(i8* getelementptr inbounds ([3 x i8], [3 x i8]* @.str, i32 0, i32 0), i8** null, i32 16) #2
+  ret i64 %call
+}
+
+define i64 @strtol_endptr_not_null() #0 {
+; CHECK-LABEL: @strtol_endptr_not_null(
+; CHECK-NEXT:    [[END:%.*]] = alloca i8*, align 4
+; CHECK-NEXT:    [[CALL:%.*]] = call i64 @strtol(i8* getelementptr inbounds ([2 x i8], [2 x i8]* @.str.1, i64 0, i64 0), i8** nonnull [[END]], i32 10)
+; CHECK-NEXT:    ret i64 [[CALL]]
+;
+  %end = alloca i8*, align 4
+  %call = call i64 @strtol(i8* getelementptr inbounds ([2 x i8], [2 x i8]* @.str.1, i32 0, i32 0), i8** %end, i32 10) #2
+  ret i64 %call
+}
+
+define i32 @atoi_test() #0 {
+; CHECK-LABEL: @atoi_test(
+; CHECK-NEXT:    ret i32 12
+;
+  %call = call i32 @atoi(i8* getelementptr inbounds ([3 x i8], [3 x i8]* @.str, i32 0, i32 0)) #4
+  ret i32 %call
+}
+
+define i64 @strtol_not_const_str(i8* %s) #0 {
+; CHECK-LABEL: @strtol_not_const_str(
+; CHECK-NEXT:    [[CALL:%.*]] = call i64 @strtol(i8* nocapture [[S:%.*]], i8** null, i32 10)
+; CHECK-NEXT:    ret i64 [[CALL]]
+;
+  %call = call i64 @strtol(i8* %s, i8** null, i32 10) #3
+  ret i64 %call
+}
+
+define i32 @atoi_not_const_str(i8* %s) #0 {
+; CHECK-LABEL: @atoi_not_const_str(
+; CHECK-NEXT:    [[CALL:%.*]] = call i32 @atoi(i8* [[S:%.*]])
+; CHECK-NEXT:    ret i32 [[CALL]]
+;
+  %call = call i32 @atoi(i8* %s) #4
+  ret i32 %call
+}
+
+define i64 @strtol_not_const_base(i32 %b) #0 {
+; CHECK-LABEL: @strtol_not_const_base(
+; CHECK-NEXT:    [[CALL:%.*]] = call i64 @strtol(i8* nocapture getelementptr inbounds ([3 x i8], [3 x i8]* @.str, i64 0, i64 0), i8** null, i32 [[B:%.*]])
+; CHECK-NEXT:    ret i64 [[CALL]]
+;
+  %call = call i64 @strtol(i8* getelementptr inbounds ([3 x i8], [3 x i8]* @.str, i32 0, i32 0), i8** null, i32 %b) #2
+  ret i64 %call
+}
+
+define i64 @strtol_long_int() #0 {
+; CHECK-LABEL: @strtol_long_int(
+; CHECK-NEXT:    ret i64 4294967296
+;
+  %call = call i64 @strtol(i8* getelementptr inbounds ([11 x i8], [11 x i8]* @.str.2, i32 0, i32 0), i8** null, i32 10) #3
+  ret i64 %call
+}