[GlobalISel] Remove TargetLowering::isConstantUnsignedBitfieldExtractLegal

[foad]

Summary

Use LegalizerInfo::isLegalOrCustom instead.

[foad]

TBH I don't understand why D99283 introduced this target lowering hook in the first place. Comments on the review say "The annoying part (for AArch64) is that the legality checks don't work with custom legalization". Did isLegalOrCustom not exist back then? Or does it not do the right thing? (I don't see any tests failing with this patch.)

[arsenm]

Legality rules can't be context dependent on the specific operands used, but IIRC here it only wants to use it if the operands are constants

[foad]

In D116807#3227570, @arsenm wrote:

Legality rules can't be context dependent on the specific operands used, but IIRC here it only wants to use it if the operands are constants

Well in practice the implementations of isConstantUnsignedBitfieldExtractLegal did exactly the same as the legal-or-custom check anyway, so it still seems redundant.

[foad]

Ping.

[arsenm]

LGTM. The handle constant cases better should probably be optimization combines

llvm/include/llvm/CodeGen/TargetLowering.h

#include "llvm/IR/ConstrainedOps.def"
   virtual bool useStackGuardXorFP() const { return false; }
 
   /// If the target has a standard stack protection check function that
   /// performs validation and error handling, returns the function. Otherwise,
   /// returns nullptr. Must be previously inserted by insertSSPDeclarations.
   /// Should be used only when getIRStackGuard returns nullptr.
   virtual Function *getSSPStackGuardCheck(const Module &M) const;
 
-  /// \returns true if a constant G_UBFX is legal on the target.
-  virtual bool isConstantUnsignedBitfieldExtractLegal(unsigned Opc, LLT Ty1,
-                                                      LLT Ty2) const {
-    return false;
-  }
-
 protected:
   Value *getDefaultSafeStackPointerLocation(IRBuilderBase &IRB,
                                             bool UseTLS) const;
 
 public:
   /// Returns the target-specific address of the unsafe stack pointer.
   virtual Value *getSafeStackPointerLocation(IRBuilderBase &IRB) const;
 

llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp

 
 /// Form a G_UBFX from "(a srl b) & mask", where b and mask are constants.
 bool CombinerHelper::matchBitfieldExtractFromAnd(
     MachineInstr &MI, std::function<void(MachineIRBuilder &)> &MatchInfo) {
   assert(MI.getOpcode() == TargetOpcode::G_AND);
   Register Dst = MI.getOperand(0).getReg();
   LLT Ty = MRI.getType(Dst);
   LLT ExtractTy = getTargetLowering().getPreferredShiftAmountTy(Ty);
-  if (!getTargetLowering().isConstantUnsignedBitfieldExtractLegal(
-          TargetOpcode::G_UBFX, Ty, ExtractTy))
+  if (LI && !LI->isLegalOrCustom({TargetOpcode::G_UBFX, {Ty, ExtractTy}}))
     return false;
 
   int64_t AndImm, LSBImm;
   Register ShiftSrc;
   const unsigned Size = Ty.getScalarSizeInBits();
   if (!mi_match(MI.getOperand(0).getReg(), MRI,
                 m_GAnd(m_OneNonDBGUse(m_GLShr(m_Reg(ShiftSrc), m_ICst(LSBImm))),
                        m_ICst(AndImm))))

 bool CombinerHelper::matchBitfieldExtractFromShrAnd(
     MachineInstr &MI, std::function<void(MachineIRBuilder &)> &MatchInfo) {
   const unsigned Opcode = MI.getOpcode();
   assert(Opcode == TargetOpcode::G_LSHR || Opcode == TargetOpcode::G_ASHR);
 
   const Register Dst = MI.getOperand(0).getReg();
   LLT Ty = MRI.getType(Dst);
   LLT ExtractTy = getTargetLowering().getPreferredShiftAmountTy(Ty);
-  if (!getTargetLowering().isConstantUnsignedBitfieldExtractLegal(
-          TargetOpcode::G_UBFX, Ty, ExtractTy))
+  if (LI && !LI->isLegalOrCustom({TargetOpcode::G_UBFX, {Ty, ExtractTy}}))
     return false;
 
   // Try to match shr (and x, c1), c2
   Register AndSrc;
   int64_t ShrAmt;
   int64_t SMask;
   if (!mi_match(Dst, MRI,
                 m_BinOp(Opcode,

llvm/lib/Target/AArch64/AArch64ISelLowering.h

private:
   // This function does not handle predicate bitcasts.
   SDValue getSVESafeBitCast(EVT VT, SDValue Op, SelectionDAG &DAG) const;
 
   // Returns the runtime value for PSTATE.SM. When the function is streaming-
   // compatible, this generates a call to __arm_sme_state.
   SDValue getPStateSM(SelectionDAG &DAG, SDValue Chain, SMEAttrs Attrs,
                       SDLoc DL, EVT VT) const;
 
-  bool isConstantUnsignedBitfieldExtractLegal(unsigned Opc, LLT Ty1,
-                                              LLT Ty2) const override;
-
   bool preferScalarizeSplat(SDNode *N) const override;
 };
 
 namespace AArch64 {
 FastISel *createFastISel(FunctionLoweringInfo &funcInfo,
                          const TargetLibraryInfo *libInfo);
 } // end namespace AArch64
 
 } // end namespace llvm
 
 #endif

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp

 bool AArch64TargetLowering::isTargetCanonicalConstantNode(SDValue Op) const {
   return Op.getOpcode() == AArch64ISD::DUP ||
          Op.getOpcode() == AArch64ISD::MOVI ||
          (Op.getOpcode() == ISD::EXTRACT_SUBVECTOR &&
           Op.getOperand(0).getOpcode() == AArch64ISD::DUP) ||
          TargetLowering::isTargetCanonicalConstantNode(Op);
 }
 
-bool AArch64TargetLowering::isConstantUnsignedBitfieldExtractLegal(
-    unsigned Opc, LLT Ty1, LLT Ty2) const {
-  return Ty1 == Ty2 && (Ty1 == LLT::scalar(32) || Ty1 == LLT::scalar(64));
-}
-
 bool AArch64TargetLowering::isComplexDeinterleavingSupported() const {
   return Subtarget->hasSVE() || Subtarget->hasSVE2() ||
          Subtarget->hasComplxNum();
 }
 
 bool AArch64TargetLowering::isComplexDeinterleavingOperationSupported(
     ComplexDeinterleavingOperation Operation, Type *Ty) const {
   auto *VTy = dyn_cast<VectorType>(Ty);

llvm/lib/Target/AMDGPU/AMDGPUISelLowering.h

uint32_t getImplicitParameterOffset(const uint64_t ExplicitKernArgSize,
                                       const ImplicitParameter Param) const;
 
   MVT getFenceOperandTy(const DataLayout &DL) const override {
     return MVT::i32;
   }
 
   AtomicExpansionKind shouldExpandAtomicRMWInIR(AtomicRMWInst *) const override;
 
-  bool isConstantUnsignedBitfieldExtractLegal(unsigned Opc, LLT Ty1,
-                                              LLT Ty2) const override;
-
   bool shouldSinkOperands(Instruction *I,
                           SmallVectorImpl<Use *> &Ops) const override;
 };
 
 namespace AMDGPUISD {
 
 enum NodeType : unsigned {
   // AMDIL ISD Opcodes

llvm/lib/Target/AMDGPU/AMDGPUISelLowering.cpp

if (auto *IntTy = dyn_cast<IntegerType>(RMW->getType())) {
         return AtomicExpansionKind::None;
     }
 
     return AtomicExpansionKind::CmpXChg;
   }
   }
 }
 
-bool AMDGPUTargetLowering::isConstantUnsignedBitfieldExtractLegal(
-    unsigned Opc, LLT Ty1, LLT Ty2) const {
-  return (Ty1 == LLT::scalar(32) || Ty1 == LLT::scalar(64)) &&
-         Ty2 == LLT::scalar(32);
-}
-
 /// Whether it is profitable to sink the operands of an
 /// Instruction I to the basic block of I.
 /// This helps using several modifiers (like abs and neg) more often.
 bool AMDGPUTargetLowering::shouldSinkOperands(
     Instruction *I, SmallVectorImpl<Use *> &Ops) const {
   using namespace PatternMatch;
 
   for (auto &Op : I->operands()) {