<div dir="ltr">Disregard the warning about the existing matchers. They're capturing the APInt value, not the Constant wrapper object. That should be safe.<br></div><div class="gmail_extra"><br><div class="gmail_quote">On Sun, Feb 18, 2018 at 11:05 AM, Sanjay Patel via llvm-commits <span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: spatel<br>
Date: Sun Feb 18 10:05:08 2018<br>
New Revision: 325466<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=325466&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project?rev=325466&view=rev</a><br>
Log:<br>
[PatternMatch, InstSimplify] enhance m_AllOnes() to ignore undef elements in vectors<br>
<br>
Loosening the matcher definition reveals a subtle bug in InstSimplify (we should not<br>
assume that because an operand constant matches that it's safe to return it as a result).<br>
<br>
So I'm making that change here too (that diff could be independent, but I'm not sure how<br>
to reveal it before the matcher change).<br>
<br>
This also seems like a good reason to *not* include matchers that capture the value.<br>
We don't want to encourage the potential misstep of propagating undef values when it's<br>
not allowed/intended.<br>
<br>
I didn't include the capture variant option here or in the related rL325437 (m_One),<br>
but it already exists for other constant matchers.<br>
<br>
<br>
Modified:<br>
llvm/trunk/include/llvm/IR/<wbr>PatternMatch.h<br>
llvm/trunk/lib/Analysis/<wbr>InstructionSimplify.cpp<br>
llvm/trunk/test/Transforms/<wbr>InstSimplify/or.ll<br>
llvm/trunk/test/Transforms/<wbr>InstSimplify/shr-nop.ll<br>
<br>
Modified: llvm/trunk/include/llvm/IR/<wbr>PatternMatch.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/IR/PatternMatch.h?rev=325466&r1=325465&r2=325466&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/llvm/trunk/include/<wbr>llvm/IR/PatternMatch.h?rev=<wbr>325466&r1=325465&r2=325466&<wbr>view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- llvm/trunk/include/llvm/IR/<wbr>PatternMatch.h (original)<br>
+++ llvm/trunk/include/llvm/IR/<wbr>PatternMatch.h Sun Feb 18 10:05:08 2018<br>
@@ -182,17 +182,6 @@ struct match_nan {<br>
/// Match an arbitrary NaN constant. This includes quiet and signalling nans.<br>
inline match_nan m_NaN() { return match_nan(); }<br>
<br>
-struct match_all_ones {<br>
- template <typename ITy> bool match(ITy *V) {<br>
- if (const auto *C = dyn_cast<Constant>(V))<br>
- return C->isAllOnesValue();<br>
- return false;<br>
- }<br>
-};<br>
-<br>
-/// Match an integer or vector with all bits set to true.<br>
-inline match_all_ones m_AllOnes() { return match_all_ones(); }<br>
-<br>
struct match_sign_mask {<br>
template <typename ITy> bool match(ITy *V) {<br>
if (const auto *C = dyn_cast<Constant>(V))<br>
@@ -337,6 +326,14 @@ template <typename Predicate> struct api<br>
//<br>
//////////////////////////////<wbr>//////////////////////////////<wbr>///////////////////<br>
<br>
+struct is_all_ones {<br>
+ bool isValue(const APInt &C) { return C.isAllOnesValue(); }<br>
+};<br>
+/// Match an integer or vector with all bits set.<br>
+inline cst_pred_ty<is_all_ones> m_AllOnes() {<br>
+ return cst_pred_ty<is_all_ones>();<br>
+}<br>
+<br>
struct is_maxsignedvalue {<br>
bool isValue(const APInt &C) { return C.isMaxSignedValue(); }<br>
};<br>
<br>
Modified: llvm/trunk/lib/Analysis/<wbr>InstructionSimplify.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/InstructionSimplify.cpp?rev=325466&r1=325465&r2=325466&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/llvm/trunk/lib/<wbr>Analysis/InstructionSimplify.<wbr>cpp?rev=325466&r1=325465&r2=<wbr>325466&view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- llvm/trunk/lib/Analysis/<wbr>InstructionSimplify.cpp (original)<br>
+++ llvm/trunk/lib/Analysis/<wbr>InstructionSimplify.cpp Sun Feb 18 10:05:08 2018<br>
@@ -1264,9 +1264,10 @@ static Value *SimplifyAShrInst(Value *Op<br>
MaxRecurse))<br>
return V;<br>
<br>
- // all ones >>a X -> all ones<br>
+ // all ones >>a X -> -1<br>
+ // Do not return Op0 because it may contain undef elements if it's a vector.<br>
if (match(Op0, m_AllOnes()))<br>
- return Op0;<br>
+ return Constant::getAllOnesValue(Op0-<wbr>>getType());<br>
<br>
// (X << A) >> A -> X<br>
Value *X;<br>
@@ -1783,21 +1784,16 @@ static Value *SimplifyOrInst(Value *Op0,<br>
return C;<br>
<br>
// X | undef -> -1<br>
- if (match(Op1, m_Undef()))<br>
+ // X | -1 = -1<br>
+ // Do not return Op1 because it may contain undef elements if it's a vector.<br>
+ if (match(Op1, m_Undef()) || match(Op1, m_AllOnes()))<br>
return Constant::getAllOnesValue(Op0-<wbr>>getType());<br>
<br>
// X | X = X<br>
- if (Op0 == Op1)<br>
- return Op0;<br>
-<br>
// X | 0 = X<br>
- if (match(Op1, m_Zero()))<br>
+ if (Op0 == Op1 || match(Op1, m_Zero()))<br>
return Op0;<br>
<br>
- // X | -1 = -1<br>
- if (match(Op1, m_AllOnes()))<br>
- return Op1;<br>
-<br>
// A | ~A = ~A | A = -1<br>
if (match(Op0, m_Not(m_Specific(Op1))) ||<br>
match(Op1, m_Not(m_Specific(Op0))))<br>
<br>
Modified: llvm/trunk/test/Transforms/<wbr>InstSimplify/or.ll<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstSimplify/or.ll?rev=325466&r1=325465&r2=325466&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/llvm/trunk/test/<wbr>Transforms/InstSimplify/or.ll?<wbr>rev=325466&r1=325465&r2=<wbr>325466&view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- llvm/trunk/test/Transforms/<wbr>InstSimplify/or.ll (original)<br>
+++ llvm/trunk/test/Transforms/<wbr>InstSimplify/or.ll Sun Feb 18 10:05:08 2018<br>
@@ -19,8 +19,7 @@ define i32 @all_ones(i32 %A) {<br>
<br>
define <3 x i8> @all_ones_vec_with_undef_elt(<<wbr>3 x i8> %A) {<br>
; CHECK-LABEL: @all_ones_vec_with_undef_elt(<br>
-; CHECK-NEXT: [[B:%.*]] = or <3 x i8> [[A:%.*]], <i8 -1, i8 undef, i8 -1><br>
-; CHECK-NEXT: ret <3 x i8> [[B]]<br>
+; CHECK-NEXT: ret <3 x i8> <i8 -1, i8 -1, i8 -1><br>
;<br>
%B = or <3 x i8> %A, <i8 -1, i8 undef, i8 -1><br>
ret <3 x i8> %B<br>
<br>
Modified: llvm/trunk/test/Transforms/<wbr>InstSimplify/shr-nop.ll<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstSimplify/shr-nop.ll?rev=325466&r1=325465&r2=325466&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/llvm/trunk/test/<wbr>Transforms/InstSimplify/shr-<wbr>nop.ll?rev=325466&r1=325465&<wbr>r2=325466&view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- llvm/trunk/test/Transforms/<wbr>InstSimplify/shr-nop.ll (original)<br>
+++ llvm/trunk/test/Transforms/<wbr>InstSimplify/shr-nop.ll Sun Feb 18 10:05:08 2018<br>
@@ -195,8 +195,7 @@ define i1 @ashr_ne_first_zero(i8 %a) {<br>
<br>
define <3 x i8> @ashr_all_ones_vec_with_undef_<wbr>elts(<3 x i8> %x, <3 x i8> %y) {<br>
; CHECK-LABEL: @ashr_all_ones_vec_with_undef_<wbr>elts(<br>
-; CHECK-NEXT: [[SH:%.*]] = ashr <3 x i8> <i8 undef, i8 -1, i8 undef>, [[Y:%.*]]<br>
-; CHECK-NEXT: ret <3 x i8> [[SH]]<br>
+; CHECK-NEXT: ret <3 x i8> <i8 -1, i8 -1, i8 -1><br>
;<br>
%sh = ashr <3 x i8> <i8 undef, i8 -1, i8 undef>, %y<br>
ret <3 x i8> %sh<br>
<br>
<br>
______________________________<wbr>_________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/llvm-commits</a><br>
</blockquote></div><br></div>