[PATCH] D26323: [LoopStrengthReduce] Don't use a DenseSet<int64_t> when we might add any valid int64_t to the set.

Sat Nov 5 11:00:25 PDT 2016

Sent from my Verizon Wireless 4G LTE DROID

On Nov 5, 2016 12:51 PM, Justin Lebar <jlebar at google.com<mailto:jlebar at google.com>> wrote:
>
> > 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).
>
> It currently says
>
> > (!KeyInfoT::isEqual(Val, EmptyKey) && !KeyInfoT::isEqual(Val, TombstoneKey) && "Empty/Tombstone value shouldn't be inserted into map!"
>
> I find this pretty clear, but maybe I've been corrupted by working
> with this stuff too much.  What would you like it to say instead?

Indeed; you're right. That seems fine.

>
> I looked through the results of
>
>   cat <(git grep -l 'Dense\(Map\|Set\)<u\?int') <(git grep -l
> 'SetVector<u\?int')
>
> in the monorepo (i.e., across all llvm projects).  I only found three
> places that looked suspicious to me:
>
> * InstProfWriter's ProfilingData map, where the keys are uint64 hash
> codes generated elsewhere (so I presume have the full range)
>
> https://github.com/llvm-project/llvm-project/blob/master/llvm/include/llvm/ProfileData/InstrProfWriter.h#L32
>
> * TableGen's IntInit intern table, which seems like it would be happy
> to intern INT64_MAX if anyone wrote that in a tablegen file.
>
> https://github.com/llvm-project/llvm-project/blob/master/llvm/lib/TableGen/Record.cpp#L399
>
> * Cross-DSO CFI seems to assume that type-ids are never UINT64_MAX or
> UINT64_MAX - 1.  I don't know if this is a safe assumption to make in
> the face of inputs that llvm didn't itself generate.
>
> None of these seems critical, or even likely to be hit in real life.
> OTOH I'm not familiar with most of the code I audited, so it's
> possible my suspicions weren't raised in places where they should have
> been.  It's also possible my grep query wasn't strong enough.  So
> maybe someone else should check this too.

I suppose we'd really need to check typedefs to be thorough, but that's harder than grepping I suppose ;)

-Hal

>
> -Justin
>
> On Sat, Nov 5, 2016 at 9:57 AM, Justin Lebar <jlebar at google.com<mailto:jlebar at google.com>> wrote:
> > This revision was automatically updated to reflect the committed changes.
> > Closed by commit rL286038: [LoopStrengthReduce] Don't use a DenseSet<int64_t> when we might add any valid… (authored by jlebar).
> >
> > Changed prior to commit:
> >   https://reviews.llvm.org/D26323?vs=76965&id=76969#toc
> >
> > Repository:
> >   rL LLVM
> >
> > https://reviews.llvm.org/D26323
> >
> > Files:
> >   llvm/trunk/lib/Transforms/Scalar/LoopStrengthReduce.cpp<http://LoopStrengthReduce.cpp>
> >   llvm/trunk/test/CodeGen/X86/loop-strength-reduce-crash.ll<http://loop-strength-reduce-crash.ll>
> >
> >
> > Index: llvm/trunk/lib/Transforms/Scalar/LoopStrengthReduce.cpp<http://LoopStrengthReduce.cpp>
> > ===================================================================
> > --- llvm/trunk/lib/Transforms/Scalar/LoopStrengthReduce.cpp<http://LoopStrengthReduce.cpp>
> > +++ llvm/trunk/lib/Transforms/Scalar/LoopStrengthReduce.cpp<http://LoopStrengthReduce.cpp>
> > @@ -1646,7 +1646,12 @@
> >    Instruction *IVIncInsertPos;
> >
> >    /// Interesting factors between use strides.
> > -  SmallSetVector<int64_t, 8> Factors;
> > +  ///
> > +  /// We explicitly use a SetVector which contains a SmallSet, instead of the
> > +  /// default, a SmallDenseSet, because we need to use the full range of
> > +  /// int64_ts, and there's currently no good way of doing that with
> > +  /// SmallDenseSet.
> > +  SetVector<int64_t, SmallVector<int64_t, 8>, SmallSet<int64_t, 8>> Factors;
> >
> >    /// Interesting use types, to facilitate truncation reuse.
> >    SmallSetVector<Type *, 4> Types;
> > Index: llvm/trunk/test/CodeGen/X86/loop-strength-reduce-crash.ll<http://loop-strength-reduce-crash.ll>
> > ===================================================================
> > --- llvm/trunk/test/CodeGen/X86/loop-strength-reduce-crash.ll<http://loop-strength-reduce-crash.ll>
> > +++ llvm/trunk/test/CodeGen/X86/loop-strength-reduce-crash.ll<http://loop-strength-reduce-crash.ll>
> > @@ -0,0 +1,24 @@
> > +; RUN: llc < %s -o /dev/null
> > +
> > +; Check that this doesn't crash (by virtue of using INT64_MAX as a constant in
> > +; the loop).
> > +
> > +target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
> > +target triple = "x86_64-apple-macosx10.12.0"
> > +
> > +define void @foo() {
> > +entry:
> > +  br label %for
> > +
> > +for:
> > +  %0 = phi i64 [ %add, %for ], [ undef, %entry ]
> > +  %next = phi i32 [ %inc, %for ], [ undef, %entry ]
> > +  store i32 %next, i32* undef, align 4
> > +  %add = add i64 %0, 9223372036854775807
> > +  %inc = add nsw i32 %next, 1
> > +  br i1 undef, label %exit, label %for
> > +
> > +exit:
> > +  store i64 %add, i64* undef
> > +  ret void
> > +}
> >
> >
>
> On Sat, Nov 5, 2016 at 9:57 AM, Justin Lebar <jlebar at google.com<mailto:jlebar at google.com>> wrote:
> > This revision was automatically updated to reflect the committed changes.
> > Closed by commit rL286038: [LoopStrengthReduce] Don't use a DenseSet<int64_t> when we might add any valid… (authored by jlebar).
> >
> > Changed prior to commit:
> >   https://reviews.llvm.org/D26323?vs=76965&id=76969#toc
> >
> > Repository:
> >   rL LLVM
> >
> > https://reviews.llvm.org/D26323
> >
> > Files:
> >   llvm/trunk/lib/Transforms/Scalar/LoopStrengthReduce.cpp<http://LoopStrengthReduce.cpp>
> >   llvm/trunk/test/CodeGen/X86/loop-strength-reduce-crash.ll<http://loop-strength-reduce-crash.ll>
> >
> >
> > Index: llvm/trunk/lib/Transforms/Scalar/LoopStrengthReduce.cpp<http://LoopStrengthReduce.cpp>
> > ===================================================================
> > --- llvm/trunk/lib/Transforms/Scalar/LoopStrengthReduce.cpp<http://LoopStrengthReduce.cpp>
> > +++ llvm/trunk/lib/Transforms/Scalar/LoopStrengthReduce.cpp<http://LoopStrengthReduce.cpp>
> > @@ -1646,7 +1646,12 @@
> >    Instruction *IVIncInsertPos;
> >
> >    /// Interesting factors between use strides.
> > -  SmallSetVector<int64_t, 8> Factors;
> > +  ///
> > +  /// We explicitly use a SetVector which contains a SmallSet, instead of the
> > +  /// default, a SmallDenseSet, because we need to use the full range of
> > +  /// int64_ts, and there's currently no good way of doing that with
> > +  /// SmallDenseSet.
> > +  SetVector<int64_t, SmallVector<int64_t, 8>, SmallSet<int64_t, 8>> Factors;
> >
> >    /// Interesting use types, to facilitate truncation reuse.
> >    SmallSetVector<Type *, 4> Types;
> > Index: llvm/trunk/test/CodeGen/X86/loop-strength-reduce-crash.ll<http://loop-strength-reduce-crash.ll>
> > ===================================================================
> > --- llvm/trunk/test/CodeGen/X86/loop-strength-reduce-crash.ll<http://loop-strength-reduce-crash.ll>
> > +++ llvm/trunk/test/CodeGen/X86/loop-strength-reduce-crash.ll<http://loop-strength-reduce-crash.ll>
> > @@ -0,0 +1,24 @@
> > +; RUN: llc < %s -o /dev/null
> > +
> > +; Check that this doesn't crash (by virtue of using INT64_MAX as a constant in
> > +; the loop).
> > +
> > +target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
> > +target triple = "x86_64-apple-macosx10.12.0"
> > +
> > +define void @foo() {
> > +entry:
> > +  br label %for
> > +
> > +for:
> > +  %0 = phi i64 [ %add, %for ], [ undef, %entry ]
> > +  %next = phi i32 [ %inc, %for ], [ undef, %entry ]
> > +  store i32 %next, i32* undef, align 4
> > +  %add = add i64 %0, 9223372036854775807
> > +  %inc = add nsw i32 %next, 1
> > +  br i1 undef, label %exit, label %for
> > +
> > +exit:
> > +  store i64 %add, i64* undef
> > +  ret void
> > +}
> >
> >
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161105/1c64bd9c/attachment.html>