define <2 x i64> @smax(<2 x i64> %a, <2 x i64> %b) { %x = call <2 x i64> @llvm.smax.v2i64(<2 x i64> %a, <2 x i64> %b) %y = call <2 x i64> @llvm.smax.v2i64(<2 x i64> %b, <2 x i64> %a) %o = or <2 x i64> %x, %y ret <2 x i64> %o } declare <2 x i64> @llvm.smax.v2i64(<2 x i64>, <2 x i64>) opt -O3 define <2 x i64> @smax(<2 x i64> %a, <2 x i64> %b) { %x = tail call <2 x i64> @llvm.smax.v2i64(<2 x i64> %a, <2 x i64> %b) %y = tail call <2 x i64> @llvm.smax.v2i64(<2 x i64> %b, <2 x i64> %a) %o = or <2 x i64> %x, %y ret <2 x i64> %o } but we should be able to recognise that %x and %y are equivalent: define <2 x i64> @smax(<2 x i64> %a, <2 x i64> %b) { %x = tail call <2 x i64> @llvm.smax.v2i64(<2 x i64> %a, <2 x i64> %b) ret <2 x i64> %x }
There are 2 parts to solving this: 1. Add logic to EarlyCSE to recognize commutable intrinsics. 2. Add canonicalization to InstCombine so a constant operand is always the 2nd op.
Weirdly, there is a Commutative property in Intrinsics.td but it seems to be getting completely ignored, as there is no corresponding attribute. Do we want to add one? It might be slightly odd in that this attribute wouldn't really be useful for anything but intrinsics.
(In reply to Nikita Popov from comment #2) > Weirdly, there is a Commutative property in Intrinsics.td but it seems to be > getting completely ignored, as there is no corresponding attribute. Do we > want to add one? It might be slightly odd in that this attribute wouldn't > really be useful for anything but intrinsics. It's weirder still because we have things like this: def int_smul_fix : Intrinsic<[llvm_anyint_ty], [LLVMMatchType<0>, LLVMMatchType<0>, llvm_i32_ty], [IntrNoMem, IntrSpeculatable, IntrWillReturn, Commutative, ImmArg<ArgIndex<2>>]>; So we should assume that for an intrinsic marked "Commutative" that we always mean only the 1st 2 args. I may be overlooking something, but I don't see any other place in the optimizer that would benefit from an attribute. Ie, we need to embed commute knowledge into transforms in instcombine anyway. If that's correct, then I think it's not worth the effort of creating a new attribute to map the property to.
Proposal: https://reviews.llvm.org/D86798
https://github.com/llvm/llvm-project/commit/096527214033772e8d80fdefd8a018b9bfa20021 With some follow-ups to SLP, FastISel, GVN: https://github.com/llvm/llvm-project/commit/af4581e8ab1648ff4df0b7fe3769160e6b9f2617 https://github.com/llvm/llvm-project/commit/2d3e12818e6e2466052db41833d1d564628ecfc9 https://github.com/llvm/llvm-project/commit/e25449ff57ca4d318d9d8602863806a7a29f23bc NewGVN needs more work to be on par with GVN. Not sure if anyone is planning to make that the default GVN. It's been sitting on the side for a long time.
These should be the last steps for commutative analysis for min/max: https://github.com/llvm/llvm-project/commit/bdd5bfd0e434637c44916fe2072b1d80fa022092 https://github.com/llvm/llvm-project/commit/2391a34f9f529705a9c7761df350e7f012cca191 I opened bug 47409 to track the newgvn enhancement and bug 47410 for early-cse.