If file stream arg is not captured and source is fopen, we could replace IO calls by unlocked IO ("_unlocked" function variants) to gain better speed,
Details
Diff Detail
Event Timeline
lib/Transforms/Utils/SimplifyLibCalls.cpp | ||
---|---|---|
131 | Maybe we could check for "main" function too and threat it as if it was a "static one"? |
Would be ok if we simplify getc to fgetc_unlocked and not getc_unlocked? Same for putc...
to gain better speed
Out of curiosity, what are the motivational cases, benchmarks?
if we know, that there is no fork or pthread_create calls in the current module.
I'm not sure this is sufficient.
I'll be super surprised if this could be done outside of a LTO build with no dynamic linking.
Motivation? I tried to "getchar and putchar" 2 MB file and using unlocked variants I got time difference around 0,1 s.
I believe there are more reasons to apply such optimizations, if possible. If we can have faster IO code, why not?
Why not sufficient?
Can you explain it more? Static function can be called only from module and if there is no fork or pthread_create, why we cant say that all static functions are not called from multiple threads/processes?
Static function can be called only from module and if there is no fork or pthread_create, why we cant say that all static functions are not called from multiple threads/processes?
And what about said static function being [indirectly] called from some non-static function?
Ah sure. So maybe somehow check if module has only static functions (+main)? Or iterate over non static functions in module and check if our static function is called from them?
So maybe somehow check if module has only static functions (+main)?
You might be able to get this to work, but there are a lot of weird edge cases, and it wouldn't be very useful in practice; most programs have more than one source code file.
The practical approach would be to use something like llvm::PointerMayBeCaptured to prove no other thread knows the value of the FILE*.
Code:
static void capt_test(void) {
FILE *f = fopen("file", "w"); char s[10]; fwrite(s, 10, 10, f);
}
int main(void) {
capt_test();
}
static bool isInSingleThread(CallInst *CI, IRBuilder<> &B) {
for (unsigned i = 0; i < CI->getNumOperands(); ++i) { Value *Op = CI->getOperand(i); if (Op->getType()->isPointerTy() && !isa<GlobalValue>(Op)) { errs() << "pty is " << i << " and " << PointerMayBeCaptured(Op, false, false) << "\n"; if (PointerMayBeCaptured(Op, false, false)) return false; } } return true;
}
I got that that pointers may be captured. Did I use it wrongly? It detects that fwrite captures that values, but can you explain a bit more "how to prove no other thread knows the value of the FILE via this API call"? Thanks
That should work, as far as I know; maybe we aren't adding the right nocapture markings to the fwrite() call?
I checked a CaptureTracking a bit and I was surprised that doesNotCapture (CS.doesNotCapture(A - B)) returned false for parameter, which is set as "No capture" in BuildLibCalls.
Any expert for BuildLibCalls/CaptureTracking?
Edit: well, I should use getArgOperand, but problem is still same...
for (unsigned i = 0; i < CI->getNumArgOperands(); ++i) { Value *Op = CI->getArgOperand(i); if (Op->getType()->isPointerTy() && !isa<GlobalValue>(Op)) { errs() << "pty is " << i << " and " << PointerMayBeCaptured(Op, false, false) << "\n"; if (PointerMayBeCaptured(Op, false, false)) return false; } }
NoCapture attributes check now works, I forgot about inferLibFuncAttributes..
But I still get a issue like:
define i32 @aa() #0 {
entry:
%call = call i32 @aac() ret i32 %call
}
; Function Attrs: noinline nounwind uwtable
define internal i32 @aac() #0 {
entry:
%getchar_unlocked = call i32 @getchar_unlocked() ret i32 %getchar_unlocked
}
Any idea how to fix it, @efriedma ?
I don't think there's any practical way to prove that no other thread will write to stdout.
It doesn't matter what code is in the module which is currently being processed; another module could do __attribute((constructor)) void f() { pthread_t t; pthread_create(&t, 0, puts, "foo"); } or something.
Then, this patch is dead, right? Or?
If there are no external functions that could call static ones which contain IO calls.. we are still safe to do this transformation, nope?
@efriedma, May I add tests for "capture detection" only for one IO call, e.g. fgetc? Since all others depends on the "isReplaceableWithUnlockedVariant" anyway..
lib/Analysis/TargetLibraryInfo.cpp | ||
---|---|---|
294 | We probably want to whitelist which operating systems these are available on, rather than blacklist; most of these aren't part of any standard. And actually, thinking about it a bit more, the "non-standard" part has other problems; a user could define their own function named fputc_unlocked, which does something different from the GNU functions. (Nothing else in the current BuildLibCalls.h hits this particular problem because those are all reserved names.) But I'm not sure if that's likely to be a problem in practice. | |
lib/Transforms/Utils/SimplifyLibCalls.cpp | ||
134 | This check doesn't buy you anything. | |
143 | This is incomplete; you also need to check that the instruction which produces the FILE* is a call to fopen, or the capture analysis won't do anything useful. Consider: FILE* foo() { return stdout; } Also, iterating over the operands like this is weird; probably want to explicitly pass in which operand is the FILE*. |
Notes were fixes and no futher comments were given... even if I pinged it.
I will check how to revert via arc so..
Edit: can you revert it? arc revert doesnt work with svn only.
This seems to have broken the sanitizer bots: http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-fast/builds/17377.
I will revert for now.
That is not how this works though.
https://llvm.org/docs/Phabricator.html#committing-a-change
Once a patch has been reviewed and approved on Phabricator it can then be committed to trunk.
I will check how to revert via arc so..
Edit: can you revert it? arc revert doesnt work with svn only.
Fixed and tested:
ThinLTO/X86/tli-nobuiltin.ll
Transforms/InstCombine/fprintf-1.ll
Transforms/InstCombine/fputs-1.ll
So it should not break the build.
I updated TargetLibraryInfoTest.cpp to fix other problem too..
but I got some problems when running ninja check-all
unittests/Transforms/Utils/CMakeFiles/UtilsTests.dir/IntegerDivision.cpp.o: In function `(anonymous namespace)::IntegerDivision_SRem64_Test::TestBody()':
IntegerDivision.cpp:(.text._ZN12_GLOBAL__N_127IntegerDivision_SRem64_Test8TestBodyEv+0x39c): undefined reference to `llvm::expandRemainder(llvm::BinaryOperator*)'
So please somebody verify this new revision too. Thanks.
Hi @lebedev.ri ,
Can you recommend me some person who is able to review this patch?
I ran test-suite, no breakages. Older notes to the patch was fixed. I think it is good to go.
Sorry about the delay, I got busy with other things.
lib/Analysis/TargetLibraryInfo.cpp | ||
---|---|---|
486 | You probably also need to check T.isAndroid(), like the code above this. (I haven't actually checked what bionic provides, but I'm guessing it doesn't have all of these.) | |
lib/Transforms/Utils/SimplifyLibCalls.cpp | ||
135 | I think you also need to check if the call is marked nobuiltin. | |
test/Transforms/InstCombine/unlocked-stdio.ll | ||
4 | It would be nice to have at least some tests which call fclose (since otherwise you're leaking the file handle). Needs some negative tests to make sure the capture checking is working correctly. |
lib/Analysis/TargetLibraryInfo.cpp | ||
---|---|---|
486 | They will be available since Android P, so I will enable this for Android P and higher https://android.googlesource.com/platform/bionic/+/master/docs/status.md |
Removed redundant check
lib/Transforms/Utils/SimplifyLibCalls.cpp | ||
---|---|---|
135 | I added check but I now see that optimizeCall checks for this also. Edit: removed nobuiltin check |
The code looks fine. But I'm a bit worried this could break in some cases. It's possible a user could define a function named fputc_unlocked, so we wouldn't accept a valid C program. And it's possible this could break some runtime instrumentation tools which haven't implemented the *_unlocked functions. Or someone could use some obscure C library on Linux which doesn't have all the glibc extensions and get surprised by a link error.
So I'm not sure what to do here; maybe get a second opinion on llvmdev?
We could check if e.g. fputc_unlocked is user defined in the current module, no?
Instrumentation tools are fixable, they would have to cover more functions but it is not a useless work - their analysis will just improve. I believe they would have to add some new cases to switch as a solution, etc.
Obscure C library: I believe if such C library, nobody would use Clang 7 on such system.
It would live in the trunk and it could be reverted in 7.0.1 if any major problems reported.
lib/Analysis/TargetLibraryInfo.cpp | ||
---|---|---|
485 | if (T.isGNUEnvironment()) ? |
lib/Analysis/TargetLibraryInfo.cpp | ||
---|---|---|
485 | @efriedma I updated patch to use the same way as we do for sincos - https://github.com/llvm-mirror/llvm/blob/af4d5d44bb6590140f7474323bbd11b78126a5be/lib/CodeGen/TargetLoweringBase.cpp#L164 |
LGTM.
We should merge this patch to see the impact of this patch on real code bases. We can still revert it if too many problems.
We're definitely seeing problems here and I'm not sure if it's safe in general. Also, before committing we'd prefer that actual committers review patches - and I'm not seeing that you're an active committer to the project. If so, could you let us know who you are?
In particular, for this patch I think it should have gone to llvm-dev first as Eli suggested.
Thanks.
-eric
! In D45736#1103697, @echristo wrote:
We're definitely seeing problems here and I'm not sure if it's safe in general....-eric
There was a llvm-dev thread. but only me, Eli and Han participated. There was a discussion that Linux && GNU check would be enough to be sure we have these unlocked functions.
Can you write here your problems?
The transformation logic is safe I think, we are sure via capture tracker that if fileptr cannot be captured & fileptr comes from fopen, we can use unlocked variants.
But I cannot do tests everywhere with every glibc versions, every XY system :)
And If almost nobody write comments (btw, I would like say thanks to Eli for advices not only in this patch) during review (and he wrote the code is OK), llvm-dev thread, we cannot do this transformation better, safer (?) ...
Ben's fix and comment on the patch were the problems I was seeing. More so that no actual committer to the project approved - no one I've spoken to knows who rja is.
So please revert it and if possible, write here problems what this caused. Maybe it is possible to fix it.
Not sure about benchmarks, if there are any for C FILE IO. But in my tests and programs I see small improvements - yes, they read and write a lot from / to files.
Would you like to remove it?
... or make it off by default and provide option to turn this on if anybody wants?
We probably want to whitelist which operating systems these are available on, rather than blacklist; most of these aren't part of any standard.
And actually, thinking about it a bit more, the "non-standard" part has other problems; a user could define their own function named fputc_unlocked, which does something different from the GNU functions. (Nothing else in the current BuildLibCalls.h hits this particular problem because those are all reserved names.) But I'm not sure if that's likely to be a problem in practice.