[llvm] r237453 - Canonicalize min/max expressions correctly.
James Molloy
James.Molloy at arm.com
Fri May 15 11:36:58 PDT 2015
Hi Alexey,
Thanks! It turns out your bot was the quickest to spot it - about 15 mins later I was inundated.
Thanks,
James
On 15 May 2015, at 19:32, Alexey Samsonov <vonosmas at gmail.com<mailto:vonosmas at gmail.com>> wrote:
Hi James,
Sending you the artifacts from the bot (preprocessed .cpp and .sh). This doesn't look like a glitch - the bot was red for several revisions after your commit, and turned back green after you reverted it.
On Fri, May 15, 2015 at 9:50 AM, James Molloy <james at jamesmolloy.co.uk<mailto:james at jamesmolloy.co.uk>> wrote:
Hi Alexey,
My commit appears to have caused a failure in your bot: http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-autoconf/builds/3827
but the stdio makes it seem like the process got killed (out of memory?)
Could you please let me know if it was indeed my patch or if it's a glitch in your bot? And if so how I could reproduce (or more ideally the .cpp/.sh files in /tmp)?
Cheers,
James
On Fri, 15 May 2015 at 17:17 James Molloy <james.molloy at arm.com<mailto:james.molloy at arm.com>> wrote:
Author: jamesm
Date: Fri May 15 11:10:59 2015
New Revision: 237453
URL: http://llvm.org/viewvc/llvm-project?rev=237453&view=rev
Log:
Canonicalize min/max expressions correctly.
This patch introduces a canonical form for min/max idioms where one operand
is extended or truncated. This often happens when the other operand is a
constant. For example:
%1 = icmp slt i32 %a, i32 0
%2 = sext i32 %a to i64
%3 = select i1 %1, i64 %2, i64 0
Would now be canonicalized into:
%1 = icmp slt i32 %a, i32 0
%2 = select i1 %1, i32 %a, i32 0
%3 = sext i32 %2 to i64
This builds upon a patch posted by David Majenemer
(https://www.marc.info/?l=llvm-commits&m=143008038714141&w=2). That pass
passively stopped instcombine from ruining canonical patterns. This
patch additionally actively makes instcombine canonicalize too.
Canonicalization of expressions involving a change in type from int->fp
or fp->int are not yet implemented.
Added:
llvm/trunk/test/Transforms/InstCombine/minmax-fold.ll
Modified:
llvm/trunk/lib/Transforms/InstCombine/InstCombineCompares.cpp
llvm/trunk/lib/Transforms/InstCombine/InstCombineSelect.cpp
llvm/trunk/lib/Transforms/InstCombine/InstructionCombining.cpp
Modified: llvm/trunk/lib/Transforms/InstCombine/InstCombineCompares.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/InstCombine/InstCombineCompares.cpp?rev=237453&r1=237452&r2=237453&view=diff
==============================================================================
--- llvm/trunk/lib/Transforms/InstCombine/InstCombineCompares.cpp (original)
+++ llvm/trunk/lib/Transforms/InstCombine/InstCombineCompares.cpp Fri May 15 11:10:59 2015
@@ -3970,6 +3970,19 @@ Instruction *InstCombiner::visitFCmpInst
}
}
+ // Test if the FCmpInst instruction is used exclusively by a select as
+ // part of a minimum or maximum operation. If so, refrain from doing
+ // any other folding. This helps out other analyses which understand
+ // non-obfuscated minimum and maximum idioms, such as ScalarEvolution
+ // and CodeGen. And in this case, at least one of the comparison
+ // operands has at least one user besides the compare (the select),
+ // which would often largely negate the benefit of folding anyway.
+ if (I.hasOneUse())
+ if (SelectInst *SI = dyn_cast<SelectInst>(*I.user_begin()))
+ if ((SI->getOperand(1) == Op0 && SI->getOperand(2) == Op1) ||
+ (SI->getOperand(2) == Op0 && SI->getOperand(1) == Op1))
+ return nullptr;
+
// Handle fcmp with constant RHS
if (Constant *RHSC = dyn_cast<Constant>(Op1)) {
if (Instruction *LHSI = dyn_cast<Instruction>(Op0))
Modified: llvm/trunk/lib/Transforms/InstCombine/InstCombineSelect.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/InstCombine/InstCombineSelect.cpp?rev=237453&r1=237452&r2=237453&view=diff
==============================================================================
--- llvm/trunk/lib/Transforms/InstCombine/InstCombineSelect.cpp (original)
+++ llvm/trunk/lib/Transforms/InstCombine/InstCombineSelect.cpp Fri May 15 11:10:59 2015
@@ -1154,18 +1154,30 @@ Instruction *InstCombiner::visitSelectIn
}
// See if we can fold the select into one of our operands.
- if (SI.getType()->isIntegerTy()) {
+ if (SI.getType()->isIntOrIntVectorTy()) {
if (Instruction *FoldI = FoldSelectIntoOp(SI, TrueVal, FalseVal))
return FoldI;
Value *LHS, *RHS, *LHS2, *RHS2;
- SelectPatternFlavor SPF = matchSelectPattern(&SI, LHS, RHS);
+ Instruction::CastOps CastOp;
+ SelectPatternFlavor SPF = matchSelectPattern(&SI, LHS, RHS, &CastOp);
- // MAX(MAX(a, b), a) -> MAX(a, b)
- // MIN(MIN(a, b), a) -> MIN(a, b)
- // MAX(MIN(a, b), a) -> a
- // MIN(MAX(a, b), a) -> a
if (SPF) {
+ // Canonicalize so that type casts are outside select patterns.
+ if (LHS->getType()->getPrimitiveSizeInBits() !=
+ SI.getType()->getPrimitiveSizeInBits()) {
+ CmpInst::Predicate Pred = getICmpPredicateForMinMax(SPF);
+ Value *Cmp = Builder->CreateICmp(Pred, LHS, RHS);
+ Value *NewSI = Builder->CreateCast(CastOp,
+ Builder->CreateSelect(Cmp, LHS, RHS),
+ SI.getType());
+ return ReplaceInstUsesWith(SI, NewSI);
+ }
+
+ // MAX(MAX(a, b), a) -> MAX(a, b)
+ // MIN(MIN(a, b), a) -> MIN(a, b)
+ // MAX(MIN(a, b), a) -> a
+ // MIN(MAX(a, b), a) -> a
if (SelectPatternFlavor SPF2 = matchSelectPattern(LHS, LHS2, RHS2))
if (Instruction *R = FoldSPFofSPF(cast<Instruction>(LHS),SPF2,LHS2,RHS2,
SI, SPF, RHS))
Modified: llvm/trunk/lib/Transforms/InstCombine/InstructionCombining.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/InstCombine/InstructionCombining.cpp?rev=237453&r1=237452&r2=237453&view=diff
==============================================================================
--- llvm/trunk/lib/Transforms/InstCombine/InstructionCombining.cpp (original)
+++ llvm/trunk/lib/Transforms/InstCombine/InstructionCombining.cpp Fri May 15 11:10:59 2015
@@ -714,6 +714,22 @@ Instruction *InstCombiner::FoldOpIntoSel
return nullptr;
}
+ // Test if a CmpInst instruction is used exclusively by a select as
+ // part of a minimum or maximum operation. If so, refrain from doing
+ // any other folding. This helps out other analyses which understand
+ // non-obfuscated minimum and maximum idioms, such as ScalarEvolution
+ // and CodeGen. And in this case, at least one of the comparison
+ // operands has at least one user besides the compare (the select),
+ // which would often largely negate the benefit of folding anyway.
+ if (auto *CI = dyn_cast<CmpInst>(SI->getCondition())) {
+ if (CI->hasOneUse()) {
+ Value *Op0 = CI->getOperand(0), *Op1 = CI->getOperand(1);
+ if ((SI->getOperand(1) == Op0 && SI->getOperand(2) == Op1) ||
+ (SI->getOperand(2) == Op0 && SI->getOperand(1) == Op1))
+ return nullptr;
+ }
+ }
+
Value *SelectTrueVal = FoldOperationIntoSelectOperand(Op, TV, this);
Value *SelectFalseVal = FoldOperationIntoSelectOperand(Op, FV, this);
@@ -723,7 +739,6 @@ Instruction *InstCombiner::FoldOpIntoSel
return nullptr;
}
-
/// FoldOpIntoPhi - Given a binary operator, cast instruction, or select which
/// has a PHI node as operand #0, see if we can fold the instruction into the
/// PHI (which is only possible if all operands to the PHI are constants).
Added: llvm/trunk/test/Transforms/InstCombine/minmax-fold.ll
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstCombine/minmax-fold.ll?rev=237453&view=auto
==============================================================================
--- llvm/trunk/test/Transforms/InstCombine/minmax-fold.ll (added)
+++ llvm/trunk/test/Transforms/InstCombine/minmax-fold.ll Fri May 15 11:10:59 2015
@@ -0,0 +1,73 @@
+; RUN: opt -S -instcombine < %s | FileCheck %s
+
+; CHECK-LABEL: @t1
+; CHECK-NEXT: icmp
+; CHECK-NEXT: select
+; CHECK-NEXT: sext
+define i64 @t1(i32 %a) {
+ ; This is the canonical form for a type-changing min/max.
+ %1 = icmp slt i32 %a, 5
+ %2 = select i1 %1, i32 %a, i32 5
+ %3 = sext i32 %2 to i64
+ ret i64 %3
+}
+
+; CHECK-LABEL: @t2
+; CHECK-NEXT: icmp
+; CHECK-NEXT: select
+; CHECK-NEXT: sext
+define i64 @t2(i32 %a) {
+ ; Check this is converted into canonical form, as above.
+ %1 = icmp slt i32 %a, 5
+ %2 = sext i32 %a to i64
+ %3 = select i1 %1, i64 %2, i64 5
+ ret i64 %3
+}
+
+; CHECK-LABEL: @t3
+; CHECK-NEXT: icmp
+; CHECK-NEXT: select
+; CHECK-NEXT: zext
+define i64 @t3(i32 %a) {
+ ; Same as @t2, with flipped operands and zext instead of sext.
+ %1 = icmp ult i32 %a, 5
+ %2 = zext i32 %a to i64
+ %3 = select i1 %1, i64 5, i64 %2
+ ret i64 %3
+}
+
+; CHECK-LABEL: @t4
+; CHECK-NEXT: icmp
+; CHECK-NEXT: select
+; CHECK-NEXT: trunc
+define i32 @t4(i64 %a) {
+ ; Same again, with trunc.
+ %1 = icmp slt i64 %a, 5
+ %2 = trunc i64 %a to i32
+ %3 = select i1 %1, i32 %2, i32 5
+ ret i32 %3
+}
+
+; CHECK-LABEL: @t5
+; CHECK-NEXT: icmp
+; CHECK-NEXT: zext
+; CHECK-NEXT: select
+define i64 @t5(i32 %a) {
+ ; Same as @t3, but with mismatched signedness between icmp and zext.
+ ; InstCombine should leave this alone.
+ %1 = icmp slt i32 %a, 5
+ %2 = zext i32 %a to i64
+ %3 = select i1 %1, i64 5, i64 %2
+ ret i64 %3
+}
+
+; CHECK-LABEL: @t6
+; CHECK-NEXT: icmp
+; CHECK-NEXT: select
+; CHECK-NEXT: sitofp
+define float @t6(i32 %a) {
+ %1 = icmp slt i32 %a, 0
+ %2 = select i1 %1, i32 %a, i32 0
+ %3 = sitofp i32 %2 to float
+ ret float %3
+}
_______________________________________________
llvm-commits mailing list
llvm-commits at cs.uiuc.edu<mailto:llvm-commits at cs.uiuc.edu>
http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
_______________________________________________
llvm-commits mailing list
llvm-commits at cs.uiuc.edu<mailto:llvm-commits at cs.uiuc.edu>
http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
--
Alexey Samsonov
vonosmas at gmail.com<mailto:vonosmas at gmail.com>
<old_test_suite-e7b725.tar.gz>
-- IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
ARM Limited, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No: 2557590
ARM Holdings plc, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No: 2548782
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150515/d7e62b7b/attachment.html>
More information about the llvm-commits
mailing list