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

[Hans Wennborg via Phabricator via llvm-commits]

hans added inline comments.


================
Comment at: lib/Transforms/Scalar/LoopIdiomRecognize.cpp:444-449
+  // 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;
----------------
bobsayshilol wrote:
> hans wrote:
> > bobsayshilol wrote:
> > > lebedev.ri wrote:
> > > > bobsayshilol wrote:
> > > > > lebedev.ri wrote:
> > > > > > What about `GlobalVariable`'s that aren't global constants?
> > > > > > 
> > > > > Can you give an example? `GlobalVariable` inherits from `Constant` since their address is constant, so my understanding was that all `GlobalVariable`s are therefore (global) `Constant`s.
> > > > I mean, is there any other `Constant` (other than this `GlobalVariable->isThreadDependent()`)
> > > > that could lead to similar miscompiles?
> > > 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?
> > 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?
> 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).
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 :-/


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69490/new/

https://reviews.llvm.org/D69490