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

Sat Nov 5 10:50:55 PDT 2016

> 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?

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.

-Justin

On Sat, Nov 5, 2016 at 9:57 AM, Justin Lebar <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
>   llvm/trunk/test/CodeGen/X86/loop-strength-reduce-crash.ll
>
>
> Index: llvm/trunk/lib/Transforms/Scalar/LoopStrengthReduce.cpp
> ===================================================================
> --- llvm/trunk/lib/Transforms/Scalar/LoopStrengthReduce.cpp
> +++ llvm/trunk/lib/Transforms/Scalar/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
> ===================================================================
> --- llvm/trunk/test/CodeGen/X86/loop-strength-reduce-crash.ll
> +++ llvm/trunk/test/CodeGen/X86/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> 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
>   llvm/trunk/test/CodeGen/X86/loop-strength-reduce-crash.ll
>
>
> Index: llvm/trunk/lib/Transforms/Scalar/LoopStrengthReduce.cpp
> ===================================================================
> --- llvm/trunk/lib/Transforms/Scalar/LoopStrengthReduce.cpp
> +++ llvm/trunk/lib/Transforms/Scalar/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
> ===================================================================
> --- llvm/trunk/test/CodeGen/X86/loop-strength-reduce-crash.ll
> +++ llvm/trunk/test/CodeGen/X86/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
> +}
>
>