I find the naming here confusing: Looking at this patch my expectation was that DefaultIntrinsic is used for intrinsics with default attributes, based on the name. Took me a while to understand it's actually the other way around. Shouldn't the Intrinsic <-> DefaultIntrinsic naming be inverted?

FWIW, when removing a redundant fold, it would be helpful to point out which other fold already handles it in the review summary.

LGTM

In D87344#2298584, @reames wrote:

If we want to generally handle icmps involving SCEVs which were not either uses of IVs (directly) or loop exits, I agree this code would be good to pull out and use.

The catch is the the IV logic only visits a small subset of icmps. If you have any form of complicated expression feeding the icmp, it gives up without every reaching it.

I'll note that I think it's seriously worth considering whether we want a transform which just visits all icmps in a loop and tries to eliminate them, but that's a broader task than this patch. :)

In D69686#2298687, @dmgreen wrote:

Hello.

We've ran into an issues with a case like this:
https://godbolt.org/z/KnvGvo

This now manages to prove that the "max" in a saturating the max(min(X, 32767), -32768) is always false. For some cpu's that's fantastic, and give a decent speed increase. But others have a ssat instruction that can do that saturation quickly, that match from a min(max(..)).
Same goes for MVE instructions where there are a lot of saturating instructions we were previously picking (even if we were not doing it optimally before).

Any ideas of a good way of re-proving that the lower bounds don't need to be checked, but in the backend during ISel?

(In this case the _only_ value that can saturate is a -32768*-32768 multiply, but the code remains and the performance change can be quite substantial. )

What is the relationship between this code and SimplifyIndvar::eliminateIVComparison()? Is this basically the same thing, just for icmps against something other than the IV itself?

To handle the various undef cases this ended up being a lot bulkier than I hoped - I'd love to hear if anyone has any suggestions on how we could use ConstantExpr/PatternMatch to simplify this as I imagine we could use such functionality in other places as we improve non-uniform vector support.

LGTM

LGTM

Thanks for putting up the review! I agree that the current testing situation is not ideal, but don't think the approach used here is moving us in the right direction.

Simplify implementation. There's no need to collect all the best ranges explicitly.

Use preference function instead of filter function, making the testing approach more broadly applicable.

There's a 40% code size increase on CMakeFiles/7zip-benchmark.dir/CPP/7zip/Crypto/Sha1.cpp.o, might be worth double checking that we're not missing some optimizations on funnel shifts.

LGTM

LGTM

Assuming it's not part of some other patch, this should also add a LangRef entry for the llvm.loop.mustprogress metadata.

Abandon this as no longer necessary for mustprogress?

Thanks for the clarification wrt callees, this LGTM now.

Thanks for the update, the new version should be correct. However, it can also be expensive, because we may loop through a phi multiple times until the depth limit is exhausted. I would suggest to follow the same implementation approach as computeKnownBits() does in https://github.com/llvm/llvm-project/blob/d2166076b882e38becf3657ea830ffd2b6a5695e/llvm/lib/Analysis/ValueTracking.cpp#L1574-L1606. Namely to perform the recursive call with MaxDepth - 1. (I would also suggest to handle your TODO right away, to have parity with the known bits code.)

This LGTM now, thanks!

In D88178#2293723, @lebedev.ri wrote:

Guess i'll just have to do it myself then.

LG with some nits

I've reverted this change as it breaks the clang build with linker errors. D88173 did not address them either.

FWIW there were code size changes with this patch, so it wasn't NFC.

Add additional test case @test_br_cmp_with_offset. Found in Rust code and not folded by IPSCCP (or anything else in LLVM).

I don't think this fully addresses the problem, in that this relies on the phi getting processed before the unreachable, which is not a given. This would have to happen prior to the main InstCombine run, at the same time we remove instructions in unreachable blocks. It's probably not worth it. I think we should fix this the direct way, by performing a replaceInstUsesWith(*Prev, UndefValue(Prev->getType()) before the eraseInstFromFunction() in the unreachable transform.

In D87188#2281093, @mstorsjo wrote:

This broke a few tests for me (generating code that now gives the fail result at runtime).

I'm not entirely sure which bit is the culprit, but the difference in output (that breaks tests) for one object file is available at https://martin.st/temp/vc1_block-diff-aarch64.txt, and https://martin.st/temp/vc1_block-diff-armv7.txt. For the aarch64 version, it looks like some conditionals are inverted, like these changes:

-	b.ge	.LBB1_124
+	b.hs	.LBB1_124

and

-	csel	w14, w15, w14, gt
+	csel	w14, w15, w14, hi

(with no seemingly related changes that would change the roles of the registers).

The input files for reproducing the issues are available at https://martin.st/temp/vc1_block-aarch64.c and https://martin.st/temp/vc1_block-armv7.c, which can be compiled with clang -target {aarch64,armv7}-w64-mingw32 -c -O2 vc1_block-{aarch64,armv7}.c.

Actually, that would be out-of-bounds for instcombine. Removing a phi operand would mean removing the incoming block too, and we don't do that in instcombine. So I think we just bail out if a value still has uses. SimplifyCFG will eventually get it.

In D87188#2277728, @spatel wrote:

LGTM - I think we should give this a try as-is (with the one-use check still there), see if anything regresses, then ease/remove the use check as a follow-on.

In D87326#2273732, @njames93 wrote:

@nikic Would you be able to see what the delta with this is https://gist.github.com/njames93/f26f159f06bda9e7ed2270adb39d9b08, should apply on top of trunk. It has the most expensive part of the grow buffer defined outline to dissuade inlining, may reduce binary size and improve performance with gcc9.3

Tested the newest version of this patch and I'm still seeing massive regressions, even larger than before.

LGTM. I think we have enough to start using them for vector intrinsics.

Now that D87571 also handles the FLT_MAX case, adjust this patch to use either NaN, Inf or FLT_MAX as the neutral element for fminnum (and same negative for fmaxnum). This means we never have to drop FMF.

Preserve flags when commuting operands, fix typos in comments.

Fix confusion between isSmallest and isLargest+isNegative.

In D87571#2270204, @spatel wrote:

LGTM. Should we common/integrate the caller functions/switch cases? Ie, we're switching on min/max opcode, but then translating that knowledge into the bool flags and then predicating on those flags instead of the opcodes.

Also fold FLT_MAX/-FLT_MAX if ninf flag is set.

Add SoftPromoteHalfRes as well. As we do expand non-fast reductions on X86, we can run into it there. The added tests were previously asserting.