Code complexity

In D94232#2498309, @sanwou01 wrote:

Turns out the failing benchmarks are due to a miscompilation with -g3 (which we add to profiled runs). The patch does seem to make that miscompilation more likely. I'll try to reduce that separately, but at least I'll have some performance numbers shortly.

Addressed review comments

Simplify code.

define i32 @src(i32 %A, i32 %B) {
%0:

%i = xor i32 %A, 4294967295
%i2 = and i32 %i, %B
%i3 = or i32 %B, %A
%i4 = xor i32 %i3, 4294967295
%i5 = or i32 %i2, %i4
ret i32 %i5

}

>

define i32 @tgt(i32 %A, i32 %B) {
%0:

%i = xor i32 %A, 4294967295
ret i32 %i

}
Transformation seems to be correct!

Rebase?

Addressed review comments

Addressed review comments.

Any comments from @jdoerfert / @nikic ?

In D93779#2498509, @dblaikie wrote:

In D93779#2498501, @lebedev.ri wrote:

Why do we even want to bend over backwards and try to handle broken code correctly,
at the cost of regressed performance for everything,
instead of just adding defensive asserts against such problems,
and letting/telling the user fix the code?

I believe the standard library supports this, so I think it's a somewhat awkward limitation to have a standard-library-like container that misses that. (of course the small-mode isn't entirely compatible with std::vector, etc anyway - so some divergence is possible)

Compile time regression is too big..

Looks ok.

Re-ping

Simplified implementation.

Is this something what you propose, @nikic ?

Seems ok to land?

A larger than expected codesize and runtime regression of Bullet benchmark (2.3%) for LTO. Maybe worth to analyse?

Just note: gcc enables -ffinite-loops with opt level -O2 and higher.

Introduce new extra bool argument to handle issue mentioned by @nikic

In D94180#2484644, @jdoerfert wrote:

Why only use GEPs? And it looks to me you only check the offset, doesn't that miscompile:
call(nonnull noundef (gep %p, 0)) which an arbitrary %p?

Avoid optimization if inner instruction is GEP.
Added test.

In D86844#2484639, @atmnpatel wrote:

In D86844#2484568, @fhahn wrote:

In D86844#2481922, @xbolva00 wrote:

int a, b;

int f(void) {
    while (1) {
        if (a != b) return 1;
    }
    return 0;
}

int g(int a, int b) {
    while (1) {
        if (a != b) return 1;
    }
    return 0;
}

LLVM does not catch these cases; gcc does.

https://godbolt.org/z/jW7son

Looks like must progress does not get added? If it gets added to the IR the loops get removed: https://godbolt.org/z/77v17P

I might be misunderstanding the standard here but since 1 is a non-zero constant expression, it can't be assumed to terminate by the implementation right? The relevant section from C11 at least is "An iteration statement whose controlling expression is not a constant expression that performs [explanation of what it deems as progress] may be assumed by the implementation to terminate" (C11 6.8.5 p6). I think these cases fall outside of the scope of this particular change ...

For the example, I think this InstCombine transformation will work. noundef isn't necessary.

Restrict to nonnull+noundef.
Handle GEPs.

int a, b;

Patch title should be adjusted I think

Reping

Do you plan to implement gcc’s option in Clang as followup?

GCC has separate pass for this - loopsplit. It can handle also general case like:

Great! So something like gcc function spliting/cloning? (It would be good to support noipa, noclone to disable this optimalization)

noreturn makes sense here

Can you reland this patch now?

Please add a test

In D30686#758530, @ABataev wrote:

Michael, tried to implement it in InstCombiner, but we need cost analysis. Later passes are not suitable for this, because we need transformed code in InstCombiner. So, reopened it.

Please add tests

Kinda big impact on code size

Suspicious code size regression (npm -03)

Performance data? No regressions for benchmarks?

Status?

Are you gonna land this patch?

Thanks!