@efriedma is it now OK? Or is there anything to improve?

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?

Better tests

Updated tests.

Fixed things .

Which additional tests you would like to see?

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.

No you can merge this patch, @spatel

@efriedma . may I add tests for "capture detection" only for one IO call, e.g. fwrite? Since all others depends on the "isReplaceableWithUnlockedVariant" anyway..

Another idea if you want to implement it - check fopen.

Shouldn't it catch in curl also this code?

Adressed notes by @efriedma

In D45736#1071437, @efriedma wrote:

It's still possible to optimize your capt_test() example.

In D45736#1071418, @efriedma wrote:

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.

In D45736#1071386, @efriedma wrote:

I don't think there's any practical way to prove that no other thread will write to stdout.

NoCapture attributes check now work.

ping

Update with the context

In D45343#1069903, @spatel wrote:

I don't understand what's going on here. Why is this patch marked 'Accepted'? In general, you shouldn't approve your own patches that you've posted for review.

Beyond that, I don't understand why this is desirable. I think the reason this was coded with size optimization as a predicate is because the null check cheaply guards against a potentially expensive library call (cc @qcolombet as author of the original patch). This might be the correct transform for instcombine, but then there should be a backend transform that would add the null check back if it's profitable?

In D45736#1070442, @efriedma wrote:

That should work, as far as I know; maybe we aren't adding the right nocapture markings to the fwrite() call?

In D45736#1070347, @efriedma wrote:

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

In D45736#1070330, @lebedev.ri wrote:

In D45736#1070328, @xbolva00 wrote:

In D45736#1070249, @lebedev.ri wrote:

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?

And what about said static function being [indirectly] called from some non-static function?

In D45736#1070249, @lebedev.ri wrote:

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.

Would be ok if we simplify getc to fgetc_unlocked and not getc_unlocked?

ping

In D45413#1068982, @efriedma wrote:

LGTM

Added tests for atol, atoll, strtoll.

Updated, fixed things.

Updated with changes requested by @efriedma .

Any review or merge? @spatel ?

@efriedma , is now everything ok?

Please update with the context.

In D45599#1066574, @dberlin wrote:

GVN should already be able to get this, and if it can't, improve it's intrinsic handling (Either in VNCoercion's memory transfer intrinsic handling, or ...).

That's gonna do a lot better than trying to special case it in libcall simplification.
It also looks like you made it O(number of instructions) worst case per call?

Updated, use strtoll now. @efriedma

Patch updated with suggested changes by @lebedev.ri

I would like to hear your feedback on these three changes

Anything to be done? Or is this ok? @efriedma , @spatel...

Fixed things noted by @efriedma

Is patch now ok for you, @efriedma ?

In D45378#1064397, @spatel wrote:

In D45378#1060270, @xbolva00 wrote:

In D45378#1060239, @spatel wrote:

This is pushing instcombine beyond local simplifications. Propagating values across blocks should probably be handled in CVP (-correlated-propagation).

Maybe, but here it also fits, every info required for this transformation is available here.

I think it's been shown that every other pass could be subsumed by instcombine. That doesn't mean we should do that.

Implementation of this optimization would probably be quite massive in the CorrelatedValuePropagation.

I was curious why that might be, so I wrote a patch. It's about the same amount of code as this patch, but more general. Please see if D45448 / rL329755 solved your motivating examples. If yes, I think you can abandon this patch.