[llvm] r286289 - GlobalISel: allow CodeGen to fallback on VReg type/class issues.

Justin Bogner via llvm-commits llvm-commits at lists.llvm.org
Wed Jan 25 12:19:34 PST 2017


Tim Northover via llvm-commits <llvm-commits at lists.llvm.org> writes:
> Author: tnorthover
> Date: Tue Nov  8 14:39:03 2016
> New Revision: 286289
>
> URL: http://llvm.org/viewvc/llvm-project?rev=286289&view=rev
> Log:
> GlobalISel: allow CodeGen to fallback on VReg type/class issues.
>
> After instruction selection we perform some checks on each VReg just before
> discarding the type information. These checks were assertions before, but that
> breaks the fallback path so this patch moves the logic into the main flow and
> reports a better error on failure.
>
> Modified:
>     llvm/trunk/include/llvm/CodeGen/MachineRegisterInfo.h
>     llvm/trunk/lib/CodeGen/GlobalISel/InstructionSelect.cpp
>     llvm/trunk/lib/CodeGen/MachineRegisterInfo.cpp
>
> Modified: llvm/trunk/include/llvm/CodeGen/MachineRegisterInfo.h
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/CodeGen/MachineRegisterInfo.h?rev=286289&r1=286288&r2=286289&view=diff
> ==============================================================================
>
> --- llvm/trunk/include/llvm/CodeGen/MachineRegisterInfo.h (original)
> +++ llvm/trunk/include/llvm/CodeGen/MachineRegisterInfo.h Tue Nov  8 14:39:03 2016
> @@ -109,14 +109,6 @@ private:
>    /// Map generic virtual registers to their actual size.
>    mutable std::unique_ptr<VRegToTypeMap> VRegToType;
>  
> -  /// Accessor for VRegToType. This accessor should only be used
> -  /// by global-isel related work.
> -  VRegToTypeMap &getVRegToType() const {
> -    if (!VRegToType)
> -      VRegToType.reset(new VRegToTypeMap);
> -    return *VRegToType.get();
> -  }
> -
>    /// Keep track of the physical registers that are live in to the function.
>    /// Live in values are typically arguments in registers.  LiveIn values are
>    /// allowed to have virtual registers associated with them, stored in the
> @@ -642,6 +634,14 @@ public:
>    ///
>    unsigned createVirtualRegister(const TargetRegisterClass *RegClass);
>  
> +  /// Accessor for VRegToType. This accessor should only be used
> +  /// by global-isel related work.
> +  VRegToTypeMap &getVRegToType() const {
> +    if (!VRegToType)
> +      VRegToType.reset(new VRegToTypeMap);
> +    return *VRegToType.get();
> +  }
> +
>    /// Get the low-level type of \p VReg or LLT{} if VReg is not a generic
>    /// (target independent) virtual register.
>    LLT getType(unsigned VReg) const;
> @@ -654,7 +654,8 @@ public:
>    unsigned createGenericVirtualRegister(LLT Ty);
>  
>    /// Remove all types associated to virtual registers (after instruction
> -  /// selection and constraining of all generic virtual registers).
> +  /// selection and constraining of all generic virtual registers). Returns true
> +  /// if the VReg mapping was consistent.
>    void clearVirtRegTypes();
>  
>    /// Creates a new virtual register that has no register class, register bank
>
> Modified: llvm/trunk/lib/CodeGen/GlobalISel/InstructionSelect.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/GlobalISel/InstructionSelect.cpp?rev=286289&r1=286288&r2=286289&view=diff
> ==============================================================================
> --- llvm/trunk/lib/CodeGen/GlobalISel/InstructionSelect.cpp (original)
> +++ llvm/trunk/lib/CodeGen/GlobalISel/InstructionSelect.cpp Tue Nov  8 14:39:03 2016
> @@ -44,11 +44,13 @@ void InstructionSelect::getAnalysisUsage
>    MachineFunctionPass::getAnalysisUsage(AU);
>  }
>  
> -static void reportSelectionError(const MachineInstr &MI, const Twine &Message) {
> -  const MachineFunction &MF = *MI.getParent()->getParent();
> +static void reportSelectionError(const MachineInstr *MI, const Twine &Message) {
> +  const MachineFunction &MF = *MI->getParent()->getParent();
>    std::string ErrStorage;
>    raw_string_ostream Err(ErrStorage);
> -  Err << Message << ":\nIn function: " << MF.getName() << '\n' << MI << '\n';
> +  Err << Message << ":\nIn function: " << MF.getName() << '\n';
> +  if (MI)
> +    Err << *MI << '\n';
>    report_fatal_error(Err.str());
>  }
>  
> @@ -80,7 +82,7 @@ bool InstructionSelect::runOnMachineFunc
>      for (const MachineBasicBlock &MBB : MF)
>        for (const MachineInstr &MI : MBB)
>          if (isPreISelGenericOpcode(MI.getOpcode()) && !MLI->isLegal(MI, MRI))
> -          reportSelectionError(MI, "Instruction is not legal");
> +          reportSelectionError(&MI, "Instruction is not legal");
>  
>  #endif
>    // FIXME: We could introduce new blocks and will need to fix the outer loop.
> @@ -113,7 +115,10 @@ bool InstructionSelect::runOnMachineFunc
>  
>        if (!ISel->select(MI)) {
>          if (TPC.isGlobalISelAbortEnabled())
> -          reportSelectionError(MI, "Cannot select");
> +          // FIXME: It would be nice to dump all inserted instructions.  It's
> +          // not
> +          // obvious how, esp. considering select() can insert after MI.
> +          reportSelectionError(&MI, "Cannot select");
>          Failed = true;
>          break;
>        }
> @@ -129,21 +134,40 @@ bool InstructionSelect::runOnMachineFunc
>      }
>    }
>  
> +  // Now that selection is complete, there are no more generic vregs.  Verify
> +  // that the size of the now-constrained vreg is unchanged and that it has a
> +  // register class.
> +  for (auto &VRegToType : MRI.getVRegToType()) {
> +    unsigned VReg = VRegToType.first;
> +    auto *RC = MRI.getRegClassOrNull(VReg);
> +    auto *MI = MRI.def_instr_begin(VReg) == MRI.def_instr_end()
> +                   ? nullptr
> +                   : &*MRI.def_instr_begin(VReg);

As far as I can tell we don't remove anything from the VRegToType map,
so we're doing these checks even when the VReg is completely dead here
This could come up when we remove a G_SEQUENCE/G_EXTRACT pair in
Legalizer::combineExtracts, for example. Does it actually make sense to
check the register class and size in those cases?

> +    if (!RC) {
> +      if (TPC.isGlobalISelAbortEnabled())
> +        reportSelectionError(MI, "VReg as no regclass after selection");
> +      Failed = true;
> +      break;
> +    }
> +
> +    if (VRegToType.second.isValid() &&
> +        VRegToType.second.getSizeInBits() > (RC->getSize() * 8)) {
> +      if (TPC.isGlobalISelAbortEnabled())
> +        reportSelectionError(
> +            MI, "VReg has explicit size different from class size");
> +      Failed = true;
> +      break;
> +    }
> +  }
> +
> +  MRI.getVRegToType().clear();
> +
>    if (!TPC.isGlobalISelAbortEnabled() && (Failed || MF.size() == NumBlocks)) {
>      MF.getProperties().set(MachineFunctionProperties::Property::FailedISel);
>      return false;
>    }
>    assert(MF.size() == NumBlocks && "Inserting blocks is not supported yet");
>  
> -  // Now that selection is complete, there are no more generic vregs.
> -  // FIXME: We're still discussing what to do with the vreg->size map:
> -  // it's somewhat redundant (with the def MIs type size), but having to
> -  // examine MIs is also awkward.  Another alternative is to track the type on
> -  // the vreg instead, but that's not ideal either, because it's saying that
> -  // vregs have types, which they really don't. But then again, LLT is just
> -  // a size and a "shape": it's probably the same information as regbank info.
> -  MF.getRegInfo().clearVirtRegTypes();
> -
>    // FIXME: Should we accurately track changes?
>    return true;
>  }
>
> Modified: llvm/trunk/lib/CodeGen/MachineRegisterInfo.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/MachineRegisterInfo.cpp?rev=286289&r1=286288&r2=286289&view=diff
> ==============================================================================
> --- llvm/trunk/lib/CodeGen/MachineRegisterInfo.cpp (original)
> +++ llvm/trunk/lib/CodeGen/MachineRegisterInfo.cpp Tue Nov  8 14:39:03 2016
> @@ -143,17 +143,6 @@ MachineRegisterInfo::createGenericVirtua
>  }
>  
>  void MachineRegisterInfo::clearVirtRegTypes() {
> -#ifndef NDEBUG
> -  // Verify that the size of the now-constrained vreg is unchanged.
> -  for (auto &VRegToType : getVRegToType()) {
> -    auto *RC = getRegClass(VRegToType.first);
> -    if (VRegToType.second.isValid() &&
> -        VRegToType.second.getSizeInBits() > (RC->getSize() * 8))
> -      llvm_unreachable(
> -          "Virtual register has explicit size different from its class size");
> -  }
> -#endif
> -
>    getVRegToType().clear();
>  }
>  
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits


More information about the llvm-commits mailing list