From 374ef57f1379d3d3bbe5bfb19f1d2ea7e79b6db9 Mon Sep 17 00:00:00 2001 From: Roman Lebedev Date: Tue, 29 Dec 2020 16:27:55 +0300 Subject: [PATCH] [InstCombine] 'hoist xor-by-constant from xor-by-value': completely give up on constant exprs MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit As Mikael Holmén is noting in the post-commit review for the first fix https://reviews.llvm.org/rGd4ccef38d0bb#967466 not hoisting constantexprs is not enough, because if the xor originally was a constantexpr (i.e. X is a constantexpr). `SimplifyAssociativeOrCommutative()` in `visitXor()` will immediately undo this transform, thus again causing an infinite combine loop. This transform has resulted in a surprising number of constantexpr failures. --- .../Transforms/InstCombine/InstCombineAndOrXor.cpp | 8 +++++--- .../hoist-xor-by-constant-from-xor-by-value.ll | 20 ++++++++++++++++++++ 2 files changed, 25 insertions(+), 3 deletions(-) diff --git a/llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp b/llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp index c0823c95..15dcf2d 100644 --- a/llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp +++ b/llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp @@ -3459,9 +3459,11 @@ Instruction *InstCombinerImpl::visitXor(BinaryOperator &I) { // Otherwise, if all else failed, try to hoist the xor-by-constant: // (X ^ C) ^ Y --> (X ^ Y) ^ C - // Just like we do in other places, we avoid the fold for constantexprs, - // at least to avoid endless combine loop. - if (match(&I, m_c_Xor(m_OneUse(m_Xor(m_Value(X), m_ImmConstant(C1))), + // Just like we do in other places, we completely avoid the fold + // for constantexprs, at least to avoid endless combine loop. + if (match(&I, m_c_Xor(m_OneUse(m_Xor(m_CombineAnd(m_Value(X), + m_Unless(m_ConstantExpr())), + m_ImmConstant(C1))), m_Value(Y)))) return BinaryOperator::CreateXor(Builder.CreateXor(X, Y), C1); diff --git a/llvm/test/Transforms/InstCombine/hoist-xor-by-constant-from-xor-by-value.ll b/llvm/test/Transforms/InstCombine/hoist-xor-by-constant-from-xor-by-value.ll index 14227d3..46bdb1a 100644 --- a/llvm/test/Transforms/InstCombine/hoist-xor-by-constant-from-xor-by-value.ll +++ b/llvm/test/Transforms/InstCombine/hoist-xor-by-constant-from-xor-by-value.ll @@ -87,3 +87,23 @@ entry: %r = xor i8 %or, xor (i8 xor (i8 ptrtoint (i32* @global_constant to i8), i8 -1), i8 xor (i8 ptrtoint (i32* @global_constant2 to i8), i8 -1)) ret i8 %r } + +@global_constant3 = external global [6 x [1 x i64]], align 1 +@global_constant4 = external global i64, align 1 +@global_constant5 = external global i16*, align 1 + +define i16 @constantexpr2() { +; CHECK-LABEL: @constantexpr2( +; CHECK-NEXT: [[I2:%.*]] = load i16*, i16** @global_constant5, align 1 +; CHECK-NEXT: [[I3:%.*]] = load i16, i16* [[I2]], align 1 +; CHECK-NEXT: [[I5:%.*]] = xor i16 [[I3]], xor (i16 zext (i1 icmp ne (i64* getelementptr inbounds ([6 x [1 x i64]], [6 x [1 x i64]]* @global_constant3, i64 0, i64 5, i64 0), i64* @global_constant4) to i16), i16 -1) +; CHECK-NEXT: ret i16 [[I5]] +; + %i0 = icmp ne i64* getelementptr inbounds ([6 x [1 x i64]], [6 x [1 x i64]]* @global_constant3, i16 0, i16 5, i16 0), @global_constant4 + %i1 = zext i1 %i0 to i16 + %i2 = load i16*, i16** @global_constant5, align 1 + %i3 = load i16, i16* %i2, align 1 + %i4 = xor i16 %i3, %i1 + %i5 = xor i16 %i4, -1 + ret i16 %i5 +} -- 2.7.4