<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body>
<div>
<div><font style="color:#333333"><i>Sent from my Verizon Wireless 4G LTE DROID</i></font></div>
<div><br>
</div>
<div>On Nov 5, 2016 12:51 PM, Justin Lebar <<a href="mailto:jlebar@google.com">jlebar@google.com</a>> wrote:</div>
<div>></div>
<div>> > We should probably also have some template specialization in the DenseMap implementation that causes us to have an assert that will catch this situation more-directly (i.e. with an easy-to-understand assertion message).</div>
<div>></div>
<div>> It currently says</div>
<div>></div>
<div>> > (!KeyInfoT::isEqual(Val, EmptyKey) && !KeyInfoT::isEqual(Val, TombstoneKey) && "Empty/Tombstone value shouldn't be inserted into map!"</div>
<div>></div>
<div>> I find this pretty clear, but maybe I've been corrupted by working</div>
<div>> with this stuff too much. What would you like it to say instead?</div>
<div><br>
</div>
<div>Indeed; you're right. That seems fine.</div>
<div><br>
</div>
<div>></div>
<div>> I looked through the results of</div>
<div>></div>
<div>> cat <(git grep -l 'Dense\(Map\|Set\)<u\?int') <(git grep -l</div>
<div>> 'SetVector<u\?int')</div>
<div>></div>
<div>> in the monorepo (i.e., across all llvm projects). I only found three</div>
<div>> places that looked suspicious to me:</div>
<div>></div>
<div>> * InstProfWriter's ProfilingData map, where the keys are uint64 hash</div>
<div>> codes generated elsewhere (so I presume have the full range)</div>
<div>></div>
<div>> <a href="https://github.com/llvm-project/llvm-project/blob/master/llvm/include/llvm/ProfileData/InstrProfWriter.h#L32">
https://github.com/llvm-project/llvm-project/blob/master/llvm/include/llvm/ProfileData/InstrProfWriter.h#L32</a></div>
<div>></div>
<div>> * TableGen's IntInit intern table, which seems like it would be happy</div>
<div>> to intern INT64_MAX if anyone wrote that in a tablegen file.</div>
<div>></div>
<div>> <a href="https://github.com/llvm-project/llvm-project/blob/master/llvm/lib/TableGen/Record.cpp#L399">
https://github.com/llvm-project/llvm-project/blob/master/llvm/lib/TableGen/Record.cpp#L399</a></div>
<div>></div>
<div>> * Cross-DSO CFI seems to assume that type-ids are never UINT64_MAX or</div>
<div>> UINT64_MAX - 1. I don't know if this is a safe assumption to make in</div>
<div>> the face of inputs that llvm didn't itself generate.</div>
<div>></div>
<div>> None of these seems critical, or even likely to be hit in real life.</div>
<div>> OTOH I'm not familiar with most of the code I audited, so it's</div>
<div>> possible my suspicions weren't raised in places where they should have</div>
<div>> been. It's also possible my grep query wasn't strong enough. So</div>
<div>> maybe someone else should check this too.</div>
<div><br>
</div>
<div>I suppose we'd really need to check typedefs to be thorough, but that's harder than grepping I suppose ;)</div>
<div><br>
</div>
<div>-Hal</div>
<div><br>
</div>
<div>></div>
<div>> -Justin</div>
<div>></div>
<div>> On Sat, Nov 5, 2016 at 9:57 AM, Justin Lebar <<a href="mailto:jlebar@google.com">jlebar@google.com</a>> wrote:</div>
<div>> > This revision was automatically updated to reflect the committed changes.</div>
<div>> > Closed by commit rL286038: [LoopStrengthReduce] Don't use a DenseSet<int64_t> when we might add any valid… (authored by jlebar).</div>
<div>> ></div>
<div>> > Changed prior to commit:</div>
<div>> > <a href="https://reviews.llvm.org/D26323?vs=76965&id=76969#toc">https://reviews.llvm.org/D26323?vs=76965&id=76969#toc</a></div>
<div>> ></div>
<div>> > Repository:</div>
<div>> > rL LLVM</div>
<div>> ></div>
<div>> > <a href="https://reviews.llvm.org/D26323">https://reviews.llvm.org/D26323</a></div>
<div>> ></div>
<div>> > Files:</div>
<div>> > llvm/trunk/lib/Transforms/Scalar/<a href="http://LoopStrengthReduce.cpp">LoopStrengthReduce.cpp</a></div>
<div>> > llvm/trunk/test/CodeGen/X86/<a href="http://loop-strength-reduce-crash.ll">loop-strength-reduce-crash.ll</a></div>
<div>> ></div>
<div>> ></div>
<div>> > Index: llvm/trunk/lib/Transforms/Scalar/<a href="http://LoopStrengthReduce.cpp">LoopStrengthReduce.cpp</a></div>
<div>> > ===================================================================</div>
<div>> > --- llvm/trunk/lib/Transforms/Scalar/<a href="http://LoopStrengthReduce.cpp">LoopStrengthReduce.cpp</a></div>
<div>> > +++ llvm/trunk/lib/Transforms/Scalar/<a href="http://LoopStrengthReduce.cpp">LoopStrengthReduce.cpp</a></div>
<div>> > @@ -1646,7 +1646,12 @@</div>
<div>> > Instruction *IVIncInsertPos;</div>
<div>> ></div>
<div>> > /// Interesting factors between use strides.</div>
<div>> > - SmallSetVector<int64_t, 8> Factors;</div>
<div>> > + ///</div>
<div>> > + /// We explicitly use a SetVector which contains a SmallSet, instead of the</div>
<div>> > + /// default, a SmallDenseSet, because we need to use the full range of</div>
<div>> > + /// int64_ts, and there's currently no good way of doing that with</div>
<div>> > + /// SmallDenseSet.</div>
<div>> > + SetVector<int64_t, SmallVector<int64_t, 8>, SmallSet<int64_t, 8>> Factors;</div>
<div>> ></div>
<div>> > /// Interesting use types, to facilitate truncation reuse.</div>
<div>> > SmallSetVector<Type *, 4> Types;</div>
<div>> > Index: llvm/trunk/test/CodeGen/X86/<a href="http://loop-strength-reduce-crash.ll">loop-strength-reduce-crash.ll</a></div>
<div>> > ===================================================================</div>
<div>> > --- llvm/trunk/test/CodeGen/X86/<a href="http://loop-strength-reduce-crash.ll">loop-strength-reduce-crash.ll</a></div>
<div>> > +++ llvm/trunk/test/CodeGen/X86/<a href="http://loop-strength-reduce-crash.ll">loop-strength-reduce-crash.ll</a></div>
<div>> > @@ -0,0 +1,24 @@</div>
<div>> > +; RUN: llc < %s -o /dev/null</div>
<div>> > +</div>
<div>> > +; Check that this doesn't crash (by virtue of using INT64_MAX as a constant in</div>
<div>> > +; the loop).</div>
<div>> > +</div>
<div>> > +target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"</div>
<div>> > +target triple = "x86_64-apple-macosx10.12.0"</div>
<div>> > +</div>
<div>> > +define void @foo() {</div>
<div>> > +entry:</div>
<div>> > + br label %for</div>
<div>> > +</div>
<div>> > +for:</div>
<div>> > + %0 = phi i64 [ %add, %for ], [ undef, %entry ]</div>
<div>> > + %next = phi i32 [ %inc, %for ], [ undef, %entry ]</div>
<div>> > + store i32 %next, i32* undef, align 4</div>
<div>> > + %add = add i64 %0, 9223372036854775807</div>
<div>> > + %inc = add nsw i32 %next, 1</div>
<div>> > + br i1 undef, label %exit, label %for</div>
<div>> > +</div>
<div>> > +exit:</div>
<div>> > + store i64 %add, i64* undef</div>
<div>> > + ret void</div>
<div>> > +}</div>
<div>> ></div>
<div>> ></div>
<div>></div>
<div>> On Sat, Nov 5, 2016 at 9:57 AM, Justin Lebar <<a href="mailto:jlebar@google.com">jlebar@google.com</a>> wrote:</div>
<div>> > This revision was automatically updated to reflect the committed changes.</div>
<div>> > Closed by commit rL286038: [LoopStrengthReduce] Don't use a DenseSet<int64_t> when we might add any valid… (authored by jlebar).</div>
<div>> ></div>
<div>> > Changed prior to commit:</div>
<div>> > <a href="https://reviews.llvm.org/D26323?vs=76965&id=76969#toc">https://reviews.llvm.org/D26323?vs=76965&id=76969#toc</a></div>
<div>> ></div>
<div>> > Repository:</div>
<div>> > rL LLVM</div>
<div>> ></div>
<div>> > <a href="https://reviews.llvm.org/D26323">https://reviews.llvm.org/D26323</a></div>
<div>> ></div>
<div>> > Files:</div>
<div>> > llvm/trunk/lib/Transforms/Scalar/<a href="http://LoopStrengthReduce.cpp">LoopStrengthReduce.cpp</a></div>
<div>> > llvm/trunk/test/CodeGen/X86/<a href="http://loop-strength-reduce-crash.ll">loop-strength-reduce-crash.ll</a></div>
<div>> ></div>
<div>> ></div>
<div>> > Index: llvm/trunk/lib/Transforms/Scalar/<a href="http://LoopStrengthReduce.cpp">LoopStrengthReduce.cpp</a></div>
<div>> > ===================================================================</div>
<div>> > --- llvm/trunk/lib/Transforms/Scalar/<a href="http://LoopStrengthReduce.cpp">LoopStrengthReduce.cpp</a></div>
<div>> > +++ llvm/trunk/lib/Transforms/Scalar/<a href="http://LoopStrengthReduce.cpp">LoopStrengthReduce.cpp</a></div>
<div>> > @@ -1646,7 +1646,12 @@</div>
<div>> > Instruction *IVIncInsertPos;</div>
<div>> ></div>
<div>> > /// Interesting factors between use strides.</div>
<div>> > - SmallSetVector<int64_t, 8> Factors;</div>
<div>> > + ///</div>
<div>> > + /// We explicitly use a SetVector which contains a SmallSet, instead of the</div>
<div>> > + /// default, a SmallDenseSet, because we need to use the full range of</div>
<div>> > + /// int64_ts, and there's currently no good way of doing that with</div>
<div>> > + /// SmallDenseSet.</div>
<div>> > + SetVector<int64_t, SmallVector<int64_t, 8>, SmallSet<int64_t, 8>> Factors;</div>
<div>> ></div>
<div>> > /// Interesting use types, to facilitate truncation reuse.</div>
<div>> > SmallSetVector<Type *, 4> Types;</div>
<div>> > Index: llvm/trunk/test/CodeGen/X86/<a href="http://loop-strength-reduce-crash.ll">loop-strength-reduce-crash.ll</a></div>
<div>> > ===================================================================</div>
<div>> > --- llvm/trunk/test/CodeGen/X86/<a href="http://loop-strength-reduce-crash.ll">loop-strength-reduce-crash.ll</a></div>
<div>> > +++ llvm/trunk/test/CodeGen/X86/<a href="http://loop-strength-reduce-crash.ll">loop-strength-reduce-crash.ll</a></div>
<div>> > @@ -0,0 +1,24 @@</div>
<div>> > +; RUN: llc < %s -o /dev/null</div>
<div>> > +</div>
<div>> > +; Check that this doesn't crash (by virtue of using INT64_MAX as a constant in</div>
<div>> > +; the loop).</div>
<div>> > +</div>
<div>> > +target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"</div>
<div>> > +target triple = "x86_64-apple-macosx10.12.0"</div>
<div>> > +</div>
<div>> > +define void @foo() {</div>
<div>> > +entry:</div>
<div>> > + br label %for</div>
<div>> > +</div>
<div>> > +for:</div>
<div>> > + %0 = phi i64 [ %add, %for ], [ undef, %entry ]</div>
<div>> > + %next = phi i32 [ %inc, %for ], [ undef, %entry ]</div>
<div>> > + store i32 %next, i32* undef, align 4</div>
<div>> > + %add = add i64 %0, 9223372036854775807</div>
<div>> > + %inc = add nsw i32 %next, 1</div>
<div>> > + br i1 undef, label %exit, label %for</div>
<div>> > +</div>
<div>> > +exit:</div>
<div>> > + store i64 %add, i64* undef</div>
<div>> > + ret void</div>
<div>> > +}</div>
<div>> ></div>
<div>> ></div>
</div>
</body>
</html>