[LoopIdiomRecognize] Avoid trying to create a pattern from the address of a thread local.

[bobsayshilol]

Summary

The address of a thread local is technically constant however its value isn't known until the thread of execution starts, so constructing a GlobalVariable containing its address doesn't make sense unless it is also thread local and initialised at thread startup. LLVM doesn't appear to have a concept of TLS init like it does for static construction and destruction (ie something similar to llvm::appendToGlobal{C,D}tors() but for TLS) and clang's TLS init code isn't available down in LLVM, so this patch takes the easy option and avoids the issue altogether by rejecting values that are thread locals.

A better demonstration of where this code breaks is with:

#include <cstdio>

thread_local int global_var;

__attribute__((noinline))
void test_func(int*(&ptrs)[2])
{
  printf("&global_var = %p\n", &global_var);
  for (auto& p : ptrs)
    p = &global_var;
}

int main()
{
  int*test[2]{};
  test_func(test);
  printf("test[0] = %p\n", test[0]);
}

which, on platforms that support memset_pattern16() and with optimisations enabled, will print something like:

&global_var = 0x7f1e51f77f3c
test[0] = 0x403de0

where any reads/writes of *test[0] would indeed crash at the mysterious address 0x403de0.

[lebedev.ri]

I'm not the best reviewer for this, and i'm not sure who is the code owner for that pass..

lib/Transforms/Scalar/LoopIdiomRecognize.cpp

444–449

What about GlobalVariable's that aren't global constants?

[bobsayshilol]

lib/Transforms/Scalar/LoopIdiomRecognize.cpp

444–449

Can you give an example? GlobalVariable inherits from Constant since their address is constant, so my understanding was that all GlobalVariables are therefore (global) Constants.

[lebedev.ri]

lib/Transforms/Scalar/LoopIdiomRecognize.cpp

444–449

I mean, is there any other Constant (other than this GlobalVariable->isThreadDependent())
that could lead to similar miscompiles?

[bobsayshilol]

lib/Transforms/Scalar/LoopIdiomRecognize.cpp

444–449

Good question! Looking at other uses of isThreadDependent() brought me to ValidLookupTableConstant() in SimplifyCFG.cpp which looks like the thing we should be doing here rather than just checking for TLS. The TLS part of the check was added in D4220 for a similar reason to this patch, so I'll add @hans in case they have any words of wisdom. Assuming that is the correct approach, where should ValidLookupTableConstant() be moved to so that it can be used here? ie is it OK to simply remove the static and declare it in the header?

[hans]

lib/Transforms/Scalar/LoopIdiomRecognize.cpp

444–449

It does sound like you want essentially the same as ValidLookupTableConstant() here. I guess moving that to the header file would work, but maybe we could find a better name and place for what it's trying to do?

I guess it's really trying to check if the constant can be represented as constant data (which can vary depending on the backend.) Maybe it could be turned into a utility function in Constant itself somehow?

[bobsayshilol]

lib/Transforms/Scalar/LoopIdiomRecognize.cpp

444–449

Yeah that sounds like the best approach, making it a method of Constant. But I'm terrible at naming things :P Does something like isCompileTimeConstant() or isValueKnownAtCompileTime() get across the meaning we're after?

Also I might be reading the comment in ARMTTIImpl::shouldBuildLookupTablesForConstant() wrong, but it sounds like that would be an appropriate name there too vs shouldBuildLookupTablesForConstant() since the relocation model is known at compile time so we'll know whether or not we can know the value of the Constant at compile time (and also because we'll need to call it and it's no longer specific to just lookup tables).

[hans]

lib/Transforms/Scalar/LoopIdiomRecognize.cpp

444–449

isCompileTimeConstant() is not a great improvement over Constant in general I think.. that's really what Constant mean, isn't it?

isValueKnownAtCompileTime() I'm not sure this is better. For many constants the real value is not known, and requires a run-time relocation.

I think a name that focuses on whether the value is suitable for a lookup table is maybe the easiest. The tricky part is that the decision also depends on TargetTransformInfo. I'm not sure we'd want that dependency from Constant :-/

[lebedev.ri]

Anyone else with an opinion on this?

@bobsayshilol if there isn't a bug in bugzilla for this, best to file one now, with 10 branching approaching

[bobsayshilol]

Ah yes sorry. My machine has been out of action for a while now, but now that I've got it working again it can't link clang (when did it start taking more than 12GB to link?). I'll have to revisit this patch when I get a new machine, which should be soon(TM), so I can test it still works as intended.

[lebedev.ri]

This review seems to be stuck/dead, consider abandoning if no longer relevant.

lib/Transforms/Scalar/LoopIdiomRecognize.cpp

static Constant *getMemSetPatternValue(Value *V, const DataLayout *DL) {
 
   // If the value isn't a constant, we can't promote it to being in a constant
   // array.  We could theoretically do a store to an alloca or something, but
   // that doesn't seem worthwhile.
   Constant *C = dyn_cast<Constant>(V);
   if (!C)
     return nullptr;
 
+  // While technically constant, the address of a thread_local GlobalVariable
+  // isn't known until runtime and would require additional tls init code to
+  // handle correctly.
+  if (GlobalVariable *GV = dyn_cast<GlobalVariable>(C))
+    if (GV->isThreadDependent())
+      return nullptr;
+
   // Only handle simple values that are a power of two bytes in size.
   uint64_t Size = DL->getTypeSizeInBits(V->getType());
   if (Size == 0 || (Size & 7) || (Size & (Size - 1)))
     return nullptr;
 
   // Don't care enough about darwin/ppc to implement this.
   if (DL->isBigEndian())
     return nullptr;

test/Transforms/LoopIdiom/thread-local.ll

+; RUN: opt -loop-idiom < %s -S | FileCheck %s
+
+target triple = "x86_64-apple-darwin10.0.0"
+
+;void test_tls(int **ptrs) {
+;  thread_local int tls_var;
+;  for (int i = 0; i < 1000; i++) {
+;    p[i] = &tls_var;
+;  }
+;}
+@tls_var = internal thread_local global i32 0, align 4
+
+define void @test_tls(i32** nocapture %ptrs) nounwind ssp {
+entry:
+  br label %for.body
+
+for.body:                                         ; preds = %entry, %for.body
+  %indvar = phi i64 [ 0, %entry ], [ %indvar.next, %for.body ]
+  %arrayidx = getelementptr i32*, i32** %ptrs, i64 %indvar
+  store i32* @tls_var, i32** %arrayidx, align 8
+  %indvar.next = add i64 %indvar, 1
+  %exitcond = icmp eq i64 %indvar.next, 1000
+  br i1 %exitcond, label %for.end, label %for.body
+
+for.end:                                          ; preds = %for.body
+  ret void
+; CHECK-LABEL: @test_tls(
+; CHECK-NOT: call void @memset_pattern
+}