make TargetMachine visible from TargetTransformInfo

[vtjnash]

Summary

This replaces D118541, based on feedback there.

Out-of-tree passes sometimes need information that is not exposed by the
existing Concepts while designing new features. The frontend passes
before here and the machine passes after here already get access
directly to this, and we already expect it to be available internally
here, so it is fairly trivial to store and expose.

[craig.topper]

llvm/include/llvm/CodeGen/BasicTTIImpl.h

244

Doesn't BasicTTIImpl already have access to TargetMachine via getTLI()->getTargetMachine()? See the implementation of isNoopAddrSpaceCast further down this file.

[vtjnash]

llvm/include/llvm/CodeGen/BasicTTIImpl.h

244

Yes, that is why the TM value is already here: because most subtypes were already required to provide it. Are you meaning that this should be virtual? I don't think an extra pointer stored per pass manager will be a memory issue.

[craig.topper]

llvm/include/llvm/CodeGen/BasicTTIImpl.h

244

I don't think I read the code very well. Ignore my comment.

[craig.topper]

It does feel like a bit of a layering violation to expose TargetMachine, which belongs to the Target library, to the Analysis layer. Analysis doesn't depend on Target. It would be considered bad practice to include TargetMachine.h in Analysis. Not sure if the same applies to forward declaration.

[ychen]

Echoing @craig.topper's comment, is it possible to move (or duplicate) what you need in TargetMachine to TTI?

[vtjnash]

TargetMachine is nearly always a required argument here, so clearly the analysis already does depend on the the TargetMachine. Sometimes the information we (aka JuliaLang) need is available in the "target-features" function attribute (which is always available), but we want to compare that against the TargetMachine's notion of the TargetFeatures during optimization. Those together are then nearly the complete set of arguments to the TargetMachine, so it seemed more logical to me to expose the TM directly, rather than separately exposing all of the information needed to reconstruct a copy of it. I don't think we should obfuscate this just for the sake of obfuscation, do you?

[craig.topper]

In D120585#3349242, @vtjnash wrote:

TargetMachine is nearly always a required argument here, so clearly the analysis already does depend on the the TargetMachine. Sometimes the information we (aka JuliaLang) need is available in the "target-features" function attribute (which is always available), but we want to compare that against the TargetMachine's notion of the TargetFeatures during optimization. Those together are then nearly the complete set of arguments to the TargetMachine, so it seemed more logical to me to expose the TM directly, rather than separately exposing all of the information needed to reconstruct a copy of it. I don't think we should obfuscate this just for the sake of obfuscation, do you?

The internals of the target implementation of TTI need TargetMachine, but it wasn't supposed to be exposed. That's why clang and opt have to create the TTI object outside of the pass manager.

Why does the TargetMachine's notion of target features matter if the "target-attribute" is present? If the "target-attribute" is present the TargetMachine features are ignored for that function.

[vtjnash]

It can matter in what ways the the "target-attribute" is different. Just grepping the source tree reveals there are several existing IR passes that depend on examining this function attribute, and are already altering their behavior based on the value. For example, it appears that the msan pass should be falling back to examining the TTI's notion of the target machine to locate the function arguments correctly, if I am reading the source comments here correctly:

https://github.com/llvm/llvm-project/blob/d56ef5ed20c5c1380c2d97e970c8fc4d4166bdb8/llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp#L4220-L4232

[craig.topper]

In D120585#3349367, @vtjnash wrote:

It can matter in what ways the the "target-attribute" is different. Just grepping the source tree reveals there are several existing IR passes that depend on examining this function attribute, and are already altering their behavior based on the value. For example, it appears that the msan pass should be falling back to examining the TTI's notion of the target machine to locate the function arguments correctly, if I am reading the source comments here correctly:

https://github.com/llvm/llvm-project/blob/d56ef5ed20c5c1380c2d97e970c8fc4d4166bdb8/llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp#L4220-L4232

When you say msan should be "falling back", do you mean when the "target-attribute" isn't present?

I agree the msan code is broken if the target-attribute isn't present. Though clang should always be emitting the attribute so it might not be an issue in practice.

I would be supportive of a patch that added a TTI function that returned the contents of the target-attribute when it is present or the TargetMachine attributes when it is not.

[vtjnash]

Hm, I am actually not 100% certain, since I think that might be a current bug now that I think about it, which was caused by failing to expose this information about the TargetMachine. It looks some passes are adding flags there (e.g. +thumb/-thumb) with the implied assumption that they should be inheriting the global TargetMachine flags for anything left unspecified, while other passes (e.g. msan) are checking only the Function's flags, with the implied assumption that the configured TargetMachine does not specify target-features, and yet other passes (MIR/Codegen) are probably assuming that the "target-features" flag should entirely replace and override the TargetMachine's feature set. These assumptions seem currently incompatible.

For example, compare the assumptions on ARM of these:
https://github.com/llvm/llvm-project/blob/d56ef5ed20c5c1380c2d97e970c8fc4d4166bdb8/llvm/lib/Transforms/IPO/LowerTypeTests.cpp#L1418-L1424
https://github.com/llvm/llvm-project/blob/d56ef5ed20c5c1380c2d97e970c8fc4d4166bdb8/llvm/lib/Target/ARM/ARMTargetMachine.cpp#L268-L273

I am not trying to solve this here, but merely provide access to the tools (TargetMachine cpu architecture and features) so that these passes–and others like them–can be fixed.

[vtjnash]

I would be supportive of a patch that added a TTI function that returned the contents of the target-attribute when it is present or the TargetMachine attributes when it is not.

It appear that if we add that function, then there only be one missing argument (CPU) that would be needed so that any user could re-allocate their own private copy of the TargetMachine (see the constructor at https://llvm.org/doxygen/classllvm_1_1TargetMachine.html). Is there some reason it is better to expose each of the values separately, but not the object that they come from?

Alternative observation to merging this PR: since the Module object already must contain the first 3 arguments to the TargetMachine constructor, would it be more logical to attach the TM directly to the module instead, replacing separate values, instead of needing the fake analysis passes like MachineModuleAnalysis to expose it to CodeGen?

llvm/include/llvm/Analysis/TargetTransformInfo.h

 class LoopInfo;
 class ProfileSummaryInfo;
 class RecurrenceDescriptor;
 class SCEV;
 class ScalarEvolution;
 class StoreInst;
 class SwitchInst;
 class TargetLibraryInfo;
+class TargetMachine;
 class Type;
 class User;
 class Value;
 class VPIntrinsic;
 struct KnownBits;
 template <typename T> class Optional;
 
 /// Information about a load/store intrinsic defined by the target.
public:
   /// This is used by targets to construct a TTI wrapping their target-specific
   /// implementation that encodes appropriate costs for their target.
   template <typename T> TargetTransformInfo(T Impl);
 
   /// Construct a baseline TTI object using a minimal implementation of
   /// the \c Concept API below.
   ///
   /// The TTI implementation will reflect the information in the DataLayout
-  /// provided if non-null.
-  explicit TargetTransformInfo(const DataLayout &DL);
+  /// provided (required), and the TM (if non-null).
+  TargetTransformInfo(const TargetMachine *TM, const DataLayout &DL);
+  // prevent this old signature from matching the template constructor above
+  // (C++ gives a much better error this way)
+  explicit TargetTransformInfo(const DataLayout &DL) = delete;
 
   // Provide move semantics.
   TargetTransformInfo(TargetTransformInfo &&Arg);
   TargetTransformInfo &operator=(TargetTransformInfo &&RHS);
 
   // We need to define the destructor out-of-line to define our sub-classes
   // out-of-line.
   ~TargetTransformInfo();
private:
 
   std::unique_ptr<Concept> TTIImpl;
 };
 
 class TargetTransformInfo::Concept {
 public:
   virtual ~Concept() = 0;
   virtual const DataLayout &getDataLayout() const = 0;
+  virtual const TargetMachine *getTargetMachine() const = 0;
   virtual InstructionCost getGEPCost(Type *PointeeType, const Value *Ptr,
                                      ArrayRef<const Value *> Operands,
                                      TTI::TargetCostKind CostKind) = 0;
   virtual unsigned getInliningThresholdMultiplier() = 0;
   virtual unsigned adjustInliningThreshold(const CallBase *CB) = 0;
   virtual int getInlinerVectorBonusPercent() = 0;
   virtual InstructionCost getMemcpyCost(const Instruction *I) = 0;
   virtual unsigned

 public:
   Model(T Impl) : Impl(std::move(Impl)) {}
   ~Model() override = default;
 
   const DataLayout &getDataLayout() const override {
     return Impl.getDataLayout();
   }
 
+  const TargetMachine *getTargetMachine() const override {
+    return Impl.getTargetMachine();
+  }
+
   InstructionCost
   getGEPCost(Type *PointeeType, const Value *Ptr,
              ArrayRef<const Value *> Operands,
              TargetTransformInfo::TargetCostKind CostKind) override {
     return Impl.getGEPCost(PointeeType, Ptr, Operands, CostKind);
   }
   unsigned getInliningThresholdMultiplier() override {
     return Impl.getInliningThresholdMultiplier();
private:
   /// mechanism they desire for constructing the TTI for a given function.
   ///
   /// FIXME: Should we really use std::function? It's relatively inefficient.
   /// It might be possible to arrange for even stateful callbacks to outlive
   /// the analysis and thus use a function_ref which would be lighter weight.
   /// This may also be less error prone as the callback is likely to reference
   /// the external TargetMachine, and that reference needs to never dangle.
   std::function<Result(const Function &)> TTICallback;
-
-  /// Helper function used as the callback in the default constructor.
-  static Result getDefaultTTI(const Function &F);
 };
 
 /// Wrapper pass for TargetTransformInfo.
 ///
 /// This pass can be constructed from a TTI object which it stores internally
 /// and is queried by passes.
 class TargetTransformInfoWrapperPass : public ImmutablePass {
   TargetIRAnalysis TIRA;

llvm/include/llvm/Analysis/TargetTransformInfoImpl.h

 #include "llvm/IR/PatternMatch.h"
 #include <utility>
 
 using namespace llvm::PatternMatch;
 
 namespace llvm {
 
 class Function;
+class TargetMachine;
 
 /// Base class for use as a mix-in that aids implementing
 /// a TargetTransformInfo-compatible class.
 class TargetTransformInfoImplBase {
 protected:
   typedef TargetTransformInfo TTI;
 
+  const TargetMachine *TM;
   const DataLayout &DL;
 
-  explicit TargetTransformInfoImplBase(const DataLayout &DL) : DL(DL) {}
+  explicit TargetTransformInfoImplBase(const TargetMachine *TM,
+                                       const DataLayout &DL)
+      : TM(TM), DL(DL) {}
 
 public:
   // Provide value semantics. MSVC requires that we spell all of these out.
   TargetTransformInfoImplBase(const TargetTransformInfoImplBase &Arg) = default;
   TargetTransformInfoImplBase(TargetTransformInfoImplBase &&Arg) : DL(Arg.DL) {}
 
   const DataLayout &getDataLayout() const { return DL; }
+  const TargetMachine *getTargetMachine() const { return TM; }
 
   InstructionCost getGEPCost(Type *PointeeType, const Value *Ptr,
                              ArrayRef<const Value *> Operands,
                              TTI::TargetCostKind CostKind) const {
     // In the basic model, we just assume that all-constant GEPs will be folded
     // into their uses via addressing modes.
     for (unsigned Idx = 0, Size = Operands.size(); Idx != Size; ++Idx)
       if (!isa<Constant>(Operands[Idx]))

 /// CRTP base class for use as a mix-in that aids implementing
 /// a TargetTransformInfo-compatible class.
 template <typename T>
 class TargetTransformInfoImplCRTPBase : public TargetTransformInfoImplBase {
 private:
   typedef TargetTransformInfoImplBase BaseT;
 
 protected:
-  explicit TargetTransformInfoImplCRTPBase(const DataLayout &DL) : BaseT(DL) {}
+  explicit TargetTransformInfoImplCRTPBase(const TargetMachine *TM,
+                                           const DataLayout &DL)
+      : BaseT(TM, DL) {}
 
 public:
   using BaseT::getGEPCost;
 
   InstructionCost getGEPCost(Type *PointeeType, const Value *Ptr,
                              ArrayRef<const Value *> Operands,
                              TTI::TargetCostKind CostKind) {
     assert(PointeeType && Ptr && "can't get GEPCost of nullptr");

llvm/include/llvm/CodeGen/BasicTTIImpl.h

if (VariableMask) {
            getCFInstrCost(Instruction::PHI, CostKind));
     }
 
     return LoadCost + PackingCost + ConditionalCost;
   }
 
 protected:
   explicit BasicTTIImplBase(const TargetMachine *TM, const DataLayout &DL)
-      : BaseT(DL) {}
+      : BaseT(TM, DL) {}
   virtual ~BasicTTIImplBase() = default;
 
   using TargetTransformInfoImplBase::DL;
 
 public:
   /// \name Scalar TTI Implementations
   /// @{
   bool allowsMisalignedMemoryAccesses(LLVMContext &Context, unsigned BitWidth,

llvm/lib/Analysis/InlineCost.cpp

InlineCostAnnotationPrinterPass::run(Function &F,
   PrintInstructionComments = true;
   std::function<AssumptionCache &(Function &)> GetAssumptionCache =
       [&](Function &F) -> AssumptionCache & {
     return FAM.getResult<AssumptionAnalysis>(F);
   };
   Module *M = F.getParent();
   ProfileSummaryInfo PSI(*M);
   DataLayout DL(M);
-  TargetTransformInfo TTI(DL);
+  TargetTransformInfo TTI(nullptr, DL);
   // FIXME: Redesign the usage of InlineParams to expand the scope of this pass.
   // In the current implementation, the type of InlineParams doesn't matter as
   // the pass serves only for verification of inliner's decisions.
   // We can add a flag which determines InlineParams for this run. Right now,
   // the default InlineParams are used.
   const InlineParams Params = llvm::getInlineParams();
   for (BasicBlock &BB : F) {
     for (Instruction &I : BB) {

llvm/lib/Analysis/TargetTransformInfo.cpp

static cl::opt<bool> EnableReduxCost("costmodel-reduxcost", cl::init(false),
                                      cl::desc("Recognize reduction patterns."));
 
 namespace {
 /// No-op implementation of the TTI interface using the utility base
 /// classes.
 ///
 /// This is used when no target specific information is available.
 struct NoTTIImpl : TargetTransformInfoImplCRTPBase<NoTTIImpl> {
-  explicit NoTTIImpl(const DataLayout &DL)
-      : TargetTransformInfoImplCRTPBase<NoTTIImpl>(DL) {}
+  explicit NoTTIImpl(const TargetMachine *TM, const DataLayout &DL)
+      : TargetTransformInfoImplCRTPBase<NoTTIImpl>(TM, DL) {}
 };
 } // namespace
 
 bool HardwareLoopInfo::canAnalyze(LoopInfo &LI) {
   // If the loop has irreducible control flow, it can not be converted to
   // Hardware loop.
   LoopBlocksRPO RPOT(L);
   RPOT.perform(&LI);
for (BasicBlock *BB : ExitingBlocks) {
     break;
   }
 
   if (!ExitBlock)
     return false;
   return true;
 }
 
-TargetTransformInfo::TargetTransformInfo(const DataLayout &DL)
-    : TTIImpl(new Model<NoTTIImpl>(NoTTIImpl(DL))) {}
+TargetTransformInfo::TargetTransformInfo(const TargetMachine *TM,
+                                         const DataLayout &DL)
+    : TTIImpl(new Model<NoTTIImpl>(NoTTIImpl(TM, DL))) {}
 
 TargetTransformInfo::~TargetTransformInfo() = default;
 
 TargetTransformInfo::TargetTransformInfo(TargetTransformInfo &&Arg)
     : TTIImpl(std::move(Arg.TTIImpl)) {}
 
 TargetTransformInfo &TargetTransformInfo::operator=(TargetTransformInfo &&RHS) {
   TTIImpl = std::move(RHS.TTIImpl);
TargetTransformInfo::getInstructionThroughput(const Instruction *I) const {
   default:
     // We don't have any information on this instruction.
     return -1;
   }
 }
 
 TargetTransformInfo::Concept::~Concept() = default;
 
+static TargetIRAnalysis::Result getDefaultTTI(const Function &F) {
+  return TargetIRAnalysis::Result(nullptr, F.getParent()->getDataLayout());
+}
+
 TargetIRAnalysis::TargetIRAnalysis() : TTICallback(&getDefaultTTI) {}
 
 TargetIRAnalysis::TargetIRAnalysis(
     std::function<Result(const Function &)> TTICallback)
     : TTICallback(std::move(TTICallback)) {}
 
 TargetIRAnalysis::Result TargetIRAnalysis::run(const Function &F,
                                                FunctionAnalysisManager &) {
   return TTICallback(F);
 }
 
 AnalysisKey TargetIRAnalysis::Key;
 
-TargetIRAnalysis::Result TargetIRAnalysis::getDefaultTTI(const Function &F) {
-  return Result(F.getParent()->getDataLayout());
-}
-
 // Register the basic pass.
 INITIALIZE_PASS(TargetTransformInfoWrapperPass, "tti",
                 "Target Transform Information", false, true)
 char TargetTransformInfoWrapperPass::ID = 0;
 
 void TargetTransformInfoWrapperPass::anchor() {}
 
 TargetTransformInfoWrapperPass::TargetTransformInfoWrapperPass()

llvm/lib/Target/Mips/MipsTargetMachine.cpp

void MipsPassConfig::addPreRegAlloc() {
   addPass(createMipsOptimizePICCallPass());
 }
 
 TargetTransformInfo
 MipsTargetMachine::getTargetTransformInfo(const Function &F) const {
   if (Subtarget->allowMixed16_32()) {
     LLVM_DEBUG(errs() << "No Target Transform Info Pass Added\n");
     // FIXME: This is no longer necessary as the TTI returned is per-function.
-    return TargetTransformInfo(F.getParent()->getDataLayout());
+    return TargetTransformInfo(this, F.getParent()->getDataLayout());
   }
 
   LLVM_DEBUG(errs() << "Target Transform Info Pass Added\n");
   return TargetTransformInfo(BasicTTIImpl(this, F));
 }
 
 // Implemented by targets that want to run passes immediately before
 // machine code is emitted.

llvm/lib/Target/TargetMachine.cpp

 
 /// Returns the optimization level: None, Less, Default, or Aggressive.
 CodeGenOpt::Level TargetMachine::getOptLevel() const { return OptLevel; }
 
 void TargetMachine::setOptLevel(CodeGenOpt::Level Level) { OptLevel = Level; }
 
 TargetTransformInfo
 TargetMachine::getTargetTransformInfo(const Function &F) const {
-  return TargetTransformInfo(F.getParent()->getDataLayout());
+  return TargetTransformInfo(this, F.getParent()->getDataLayout());
 }
 
 void TargetMachine::getNameWithPrefix(SmallVectorImpl<char> &Name,
                                       const GlobalValue *GV, Mangler &Mang,
                                       bool MayAlwaysUsePrivate) const {
   if (MayAlwaysUsePrivate || !GV->hasPrivateLinkage()) {
     // Simple case: If GV is not private, it is not important to find out if
     // private labels are legal in this case or not.

llvm/tools/bugpoint/CrashDebugger.cpp

 
 class ReduceSimplifyCFG : public ListReducer<const BasicBlock *> {
   BugDriver &BD;
   BugTester TestFn;
   TargetTransformInfo TTI;
 
 public:
   ReduceSimplifyCFG(BugDriver &bd, BugTester testFn)
-      : BD(bd), TestFn(testFn), TTI(bd.getProgram().getDataLayout()) {}
+      : BD(bd), TestFn(testFn), TTI(nullptr, bd.getProgram().getDataLayout()) {}
 
   Expected<TestResult> doTest(std::vector<const BasicBlock *> &Prefix,
                               std::vector<const BasicBlock *> &Kept) override {
     if (!Kept.empty() && TestBlocks(Kept))
       return KeepSuffix;
     if (!Prefix.empty() && TestBlocks(Prefix))
       return KeepPrefix;
     return NoFailure;

llvm/unittests/Transforms/Utils/LocalTest.cpp

std::unique_ptr<Module> M = parseIR(Ctx, R"(
     if.bb2:
       ret i32 %1
     then.bb2:
       ret i32 0
     }
   )");
 
   Function &F = *cast<Function>(M->getNamedValue("foo"));
-  TargetTransformInfo TTI(M->getDataLayout());
+  TargetTransformInfo TTI(nullptr, M->getDataLayout());
 
   SimplifyCFGOptions Options{};
   Options.setAssumptionCache(nullptr);
 
   // Obtain BasicBlock of interest to this test, %test.bb.
   BasicBlock *TestBB = nullptr;
   for (BasicBlock &BB : F) {
     if (BB.getName().equals("test.bb")) {

llvm/unittests/Transforms/Utils/LoopRotationUtilsTest.cpp

deopt.exit:
   ret i32 %deopt.val
 })"
     );
 
   auto *F = M->getFunction("test");
   DominatorTree DT(*F);
   LoopInfo LI(DT);
   AssumptionCache AC(*F);
-  TargetTransformInfo TTI(M->getDataLayout());
+  TargetTransformInfo TTI(nullptr, M->getDataLayout());
   TargetLibraryInfoImpl TLII;
   TargetLibraryInfo TLI(TLII);
   ScalarEvolution SE(*F, TLI, AC, DT, LI);
   SimplifyQuery SQ(M->getDataLayout());
 
   Loop *L = *LI.begin();
 
   bool ret = LoopRotation(L, &LI, &TTI,
deopt.exit:
   ret i32 %deopt.val
 })"
     );
 
   auto *F = M->getFunction("test_nondup");
   DominatorTree DT(*F);
   LoopInfo LI(DT);
   AssumptionCache AC(*F);
-  TargetTransformInfo TTI(M->getDataLayout());
+  TargetTransformInfo TTI(nullptr, M->getDataLayout());
   TargetLibraryInfoImpl TLII;
   TargetLibraryInfo TLI(TLII);
   ScalarEvolution SE(*F, TLI, AC, DT, LI);
   SimplifyQuery SQ(M->getDataLayout());
 
   Loop *L = *LI.begin();
 
   bool ret = LoopRotation(L, &LI, &TTI,
                           &AC, &DT,
                           &SE, nullptr,
                           SQ, true, -1, false);
   /// LoopRotation should properly report "true" as we still perform the first rotation
   /// so we do change the IR.
   EXPECT_TRUE(ret);
 }