[llvm] r301841 - [AArch64] Move GISel accessor initialization from TargetMachine to Subtarget.

[Quentin Colombet via llvm-commits]

+Ahmed

> On May 4, 2017, at 9:36 PM, Eric Christopher <echristo at gmail.com> wrote:
> 
> 
> 
> On Mon, May 1, 2017 at 3:06 PM Quentin Colombet via llvm-commits <llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>> wrote:
> Author: qcolombet
> Date: Mon May  1 16:53:19 2017
> New Revision: 301841
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=301841&view=rev <http://llvm.org/viewvc/llvm-project?rev=301841&view=rev>
> Log:
> [AArch64] Move GISel accessor initialization from TargetMachine to Subtarget.
> 
> NFC
> 
> 
> Thanks Quentin.
> 
> One inline post-commit review change on the original:
>  
> +  // FIXME: At this point, we can't rely on Subtarget having RBI.
> +  // It's awkward to mix passing RBI and the Subtarget; should we pass
> +  // TII/TRI as well?
> 
> This seems like an odd series of comments - especially with creating RBI above. Is there a GISel target that doesn't have RBI or are you using something else?

Not that I am aware off.

> Otherwise I'm not sure what the comment is getting us :)

Ahmed, could you comment on this? I admit I am as puzzled as Eric here.

Cheers,
-Quentin

> 
> Suppose you could also have the accessor return a pointer which could be null if you wanted to conditionalize it?
> 
> *shrug*
> 
> -eric
>  
> +  AArch64GISel->InstSelector.reset(createAArch64InstructionSelector(
> +      *static_cast<const AArch64TargetMachine *>(&TM), *this, *RBI));
> +
> +  AArch64GISel->RegBankInfo.reset(RBI);
> +#endif
> +  setGISelAccessor(*AArch64GISel);
> +}
> 
>  const CallLowering *AArch64Subtarget::getCallLowering() const {
>    assert(GISel && "Access to GlobalISel APIs not set");
> 
> Modified: llvm/trunk/lib/Target/AArch64/AArch64TargetMachine.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/AArch64/AArch64TargetMachine.cpp?rev=301841&r1=301840&r2=301841&view=diff <http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/AArch64/AArch64TargetMachine.cpp?rev=301841&r1=301840&r2=301841&view=diff>
> ==============================================================================
> --- llvm/trunk/lib/Target/AArch64/AArch64TargetMachine.cpp (original)
> +++ llvm/trunk/lib/Target/AArch64/AArch64TargetMachine.cpp Mon May  1 16:53:19 2017
> @@ -11,12 +11,7 @@
>  //===----------------------------------------------------------------------===//
> 
>  #include "AArch64.h"
> -#include "AArch64CallLowering.h"
> -#include "AArch64LegalizerInfo.h"
>  #include "AArch64MacroFusion.h"
> -#ifdef LLVM_BUILD_GLOBAL_ISEL
> -#include "AArch64RegisterBankInfo.h"
> -#endif
>  #include "AArch64Subtarget.h"
>  #include "AArch64TargetMachine.h"
>  #include "AArch64TargetObjectFile.h"
> @@ -25,7 +20,6 @@
>  #include "llvm/ADT/STLExtras.h"
>  #include "llvm/ADT/Triple.h"
>  #include "llvm/Analysis/TargetTransformInfo.h"
> -#include "llvm/CodeGen/GlobalISel/GISelAccessor.h"
>  #include "llvm/CodeGen/GlobalISel/IRTranslator.h"
>  #include "llvm/CodeGen/GlobalISel/InstructionSelect.h"
>  #include "llvm/CodeGen/GlobalISel/Legalizer.h"
> @@ -222,35 +216,6 @@ AArch64TargetMachine::AArch64TargetMachi
> 
>  AArch64TargetMachine::~AArch64TargetMachine() = default;
> 
> -#ifdef LLVM_BUILD_GLOBAL_ISEL
> -namespace {
> -
> -struct AArch64GISelActualAccessor : public GISelAccessor {
> -  std::unique_ptr<CallLowering> CallLoweringInfo;
> -  std::unique_ptr<InstructionSelector> InstSelector;
> -  std::unique_ptr<LegalizerInfo> Legalizer;
> -  std::unique_ptr<RegisterBankInfo> RegBankInfo;
> -
> -  const CallLowering *getCallLowering() const override {
> -    return CallLoweringInfo.get();
> -  }
> -
> -  const InstructionSelector *getInstructionSelector() const override {
> -    return InstSelector.get();
> -  }
> -
> -  const LegalizerInfo *getLegalizerInfo() const override {
> -    return Legalizer.get();
> -  }
> -
> -  const RegisterBankInfo *getRegBankInfo() const override {
> -    return RegBankInfo.get();
> -  }
> -};
> -
> -} // end anonymous namespace
> -#endif
> -
>  const AArch64Subtarget *
>  AArch64TargetMachine::getSubtargetImpl(const Function &F) const {
>    Attribute CPUAttr = F.getFnAttribute("target-cpu");
> @@ -274,26 +239,6 @@ AArch64TargetMachine::getSubtargetImpl(c
>      resetTargetOptions(F);
>      I = llvm::make_unique<AArch64Subtarget>(TargetTriple, CPU, FS, *this,
>                                              isLittle, ForCodeSize);
> -#ifndef LLVM_BUILD_GLOBAL_ISEL
> -    GISelAccessor *GISel = new GISelAccessor();
> -#else
> -    AArch64GISelActualAccessor *GISel =
> -        new AArch64GISelActualAccessor();
> -    GISel->CallLoweringInfo.reset(
> -        new AArch64CallLowering(*I->getTargetLowering()));
> -    GISel->Legalizer.reset(new AArch64LegalizerInfo());
> -
> -    auto *RBI = new AArch64RegisterBankInfo(*I->getRegisterInfo());
> -
> -    // FIXME: At this point, we can't rely on Subtarget having RBI.
> -    // It's awkward to mix passing RBI and the Subtarget; should we pass
> -    // TII/TRI as well?
> -    GISel->InstSelector.reset(
> -        createAArch64InstructionSelector(*this, *I, *RBI));
> -
> -    GISel->RegBankInfo.reset(RBI);
> -#endif
> -    I->setGISelAccessor(*GISel);
>    }
>    return I.get();
>  }
> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits <http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170508/cca4449a/attachment-0001.html>