[InstCombine] Unlocked IO transformation - off by default

[xbolva00]

Summary

+ option to enable it

Info about transformation: https://reviews.llvm.org/D45736

[lebedev.ri]

Sadly, i'm not seeing much answers to the questions @vsk raised

I'd like to see a fuller justification here. Could you elaborate?

• What does the typical improvement look like?

• How common is this pattern is in the open source programs you looked at?

• What do you think the policy should be for simplifying libcalls in a way

that can break interceptors?

Could you CC the reviewer, rja (Joe) on this thread? They may have some
insight. I can't find their email address. They appear to have stopped
commenting on Phab shortly after this commit landed.

[xbolva00]

I sent it under r332452 but anyway..

Typical case is

void log(data) {

 file = fopen(....)
 for (...) 
     fwrite(data ..)
... maybe some more logging ...
close(file);

} // cond: file does not escape

Here we replace e.g. fwrite (in this example) (which needs to lock and unlock internal lock to be thread safe) with fwrite_unlocked which does not do expensive lock/unlock.
I tried several small open source projects and I got several hits.

• What does the typical improvement look like? Avoid useless locking in I/O functions, faster output to files.
• What do you think the policy should be for simplifying libcalls in a way that can break interceptors?

-> off by default + option to enable. Maybe interceptors can be extended in the future to handle this transformation/function calls?

Any ideas for option name? Is this one ok?

lib/Transforms/Utils/SimplifyLibCalls.cpp

 using namespace PatternMatch;
 
 static cl::opt<bool>
     EnableUnsafeFPShrink("enable-double-float-shrink", cl::Hidden,
                          cl::init(false),
                          cl::desc("Enable unsafe double to float "
                                   "shrinking for math lib calls"));
 
+static cl::opt<bool>
+    EnableUnlockedIO("enable-io-unlocked", cl::Hidden,
+                         cl::init(false),
+                         cl::desc("Enable transformation to unlocked IO lib calls"));
 
 //===----------------------------------------------------------------------===//
 // Helper Functions
 //===----------------------------------------------------------------------===//
 
 static bool ignoreCallingConv(LibFunc Func) {
   return Func == LibFunc_abs || Func == LibFunc_labs ||
          Func == LibFunc_llabs || Func == LibFunc_strlen;
static Value *convertStrToNumber(CallInst *CI, StringRef &Str, int64_t Base) {
   if (!isIntN(CI->getType()->getPrimitiveSizeInBits(), Result))
     return nullptr;
 
   return ConstantInt::get(CI->getType(), Result);
 }
 
 static bool isLocallyOpenedFile(Value *File, CallInst *CI, IRBuilder<> &B,
                                 const TargetLibraryInfo *TLI) {
+  if (!EnableUnlockedIO)
+    return false;
+
   CallInst *FOpen = dyn_cast<CallInst>(File);
   if (!FOpen)
     return false;
 
   Function *InnerCallee = FOpen->getCalledFunction();
   if (!InnerCallee)
     return false;
 

test/Transforms/InstCombine/no-unlocked-stdio.ll

+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt < %s -instcombine -S -mtriple=x86_64-unknown-linux-gnu | FileCheck %s
+
+%struct._iobuf = type { i8*, i32, i8*, i32, i32, i32, i32, i8* }
+
+@.str = private unnamed_addr constant [5 x i8] c"file\00", align 1
+@.str.1 = private unnamed_addr constant [2 x i8] c"w\00", align 1
+
+define void @external_fputc_test() {
+; CHECK-LABEL: @external_fputc_test(
+; CHECK-NEXT:    [[CALL:%.*]] = call %struct._iobuf* @fopen(i8* getelementptr inbounds ([5 x i8], [5 x i8]* @.str, i64 0, i64 0), i8* getelementptr inbounds ([2 x i8], [2 x i8]* @.str.1, i64 0, i64 0))
+; CHECK-NEXT:    [[CALL1:%.*]] = call i32 @fputc(i32 99, %struct._iobuf* [[CALL]])
+; CHECK-NEXT:    ret void
+;
+  %call = call %struct._iobuf* @fopen(i8* getelementptr inbounds ([5 x i8], [5 x i8]* @.str, i64 0, i64 0), i8* getelementptr inbounds ([2 x i8], [2 x i8]* @.str.1, i64 0, i64 0))
+  %call1 = call i32 @fputc(i32 99, %struct._iobuf* %call)
+  ret void
+}
+
+declare %struct._iobuf* @fopen(i8*, i8*)
+declare i32 @fputc(i32, %struct._iobuf* nocapture)

test/Transforms/InstCombine/unlocked-stdio-mingw.ll

 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
-; RUN: opt < %s -instcombine -S -mtriple=x86_64-w64-mingw32 | FileCheck %s
+; RUN: opt < %s -instcombine -S -enable-io-unlocked -mtriple=x86_64-w64-mingw32 | FileCheck %s
 
 %struct._iobuf = type { i8*, i32, i8*, i32, i32, i32, i32, i8* }
 
 @.str = private unnamed_addr constant [5 x i8] c"file\00", align 1
 @.str.1 = private unnamed_addr constant [2 x i8] c"w\00", align 1
 
 ; Check that this still uses the plain fputc instead of fputc_unlocked
 ; for MinGW targets.

test/Transforms/InstCombine/unlocked-stdio.ll

 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
-; RUN: opt < %s -instcombine -S -mtriple=x86_64-unknown-linux-gnu | FileCheck %s
+; RUN: opt < %s -instcombine -enable-io-unlocked -S -mtriple=x86_64-unknown-linux-gnu | FileCheck %s
 
 %struct._IO_FILE = type { i32, i8*, i8*, i8*, i8*, i8*, i8*, i8*, i8*, i8*, i8*, i8*, %struct._IO_marker*, %struct._IO_FILE*, i32, i32, i64, i16, i8, [1 x i8], i8*, i64, i8*, i8*, i8*, i8*, i64, i32, [20 x i8] }
 %struct._IO_marker = type { %struct._IO_marker*, %struct._IO_FILE*, i32 }
 
 @.str = private unnamed_addr constant [5 x i8] c"file\00", align 1
 @.str.1 = private unnamed_addr constant [2 x i8] c"w\00", align 1
 @.str.2 = private unnamed_addr constant [4 x i8] c"str\00", align 1
 @stdout = external global %struct._IO_FILE*, align 8