[GISel]: Make the combiner check if shifts are legal before creating them

[aditya_nandakumar]

Summary

Make sure shifts are legal/specified by the legalizerinfo before creating it (in the LegalizerCombiner)

[rovka]

LGTM with nit.

include/llvm/CodeGen/GlobalISel/LegalizerCombiner.h

71 ↗

(On Diff #120153)

You should check the same thing for G_AND and G_CONSTANT.

[aditya_nandakumar]

include/llvm/CodeGen/GlobalISel/LegalizerCombiner.h

71 ↗

(On Diff #120153)

Thanks. Will do it in the commit.

[rovka]

llvm/trunk/include/llvm/CodeGen/GlobalISel/LegalizerCombiner.h

98

You forgot this G_CONSTANT :)

[aditya_nandakumar]

llvm/trunk/include/llvm/CodeGen/GlobalISel/LegalizerCombiner.h

98

Fixed - r316694
Thanks

llvm/trunk/include/llvm/CodeGen/GlobalISel/LegalizerCombiner.h

 //===-- llvm/CodeGen/GlobalISel/LegalizerCombiner.h --===========//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 // This file contains some helper functions which try to cleanup artifacts
 // such as G_TRUNCs/G_[ZSA]EXTENDS that were created during legalization to make
 // the types match. This file also contains some combines of merges that happens
 // at the end of the legalization.
 //===----------------------------------------------------------------------===//
 
 #include "llvm/CodeGen/GlobalISel/Legalizer.h"
+#include "llvm/CodeGen/GlobalISel/LegalizerInfo.h"
 #include "llvm/CodeGen/GlobalISel/MachineIRBuilder.h"
 #include "llvm/CodeGen/GlobalISel/Utils.h"
 #include "llvm/CodeGen/MachineRegisterInfo.h"
 #include "llvm/Support/Debug.h"
 
 #define DEBUG_TYPE "legalizer"
 
 namespace llvm {
 class LegalizerCombiner {
   MachineIRBuilder &Builder;
   MachineRegisterInfo &MRI;
+  const LegalizerInfo &LI;
 
 public:
-  LegalizerCombiner(MachineIRBuilder &B, MachineRegisterInfo &MRI)
-      : Builder(B), MRI(MRI) {}
+  LegalizerCombiner(MachineIRBuilder &B, MachineRegisterInfo &MRI,
+                    const LegalizerInfo &LI)
+      : Builder(B), MRI(MRI), LI(LI) {}
 
   bool tryCombineAnyExt(MachineInstr &MI,
                         SmallVectorImpl<MachineInstr *> &DeadInsts) {
     if (MI.getOpcode() != TargetOpcode::G_ANYEXT)
       return false;
     MachineInstr *DefMI = MRI.getVRegDef(MI.getOperand(1).getReg());
     if (DefMI->getOpcode() == TargetOpcode::G_TRUNC) {
       DEBUG(dbgs() << ".. Combine MI: " << MI;);
public:
 
   bool tryCombineZExt(MachineInstr &MI,
                       SmallVectorImpl<MachineInstr *> &DeadInsts) {
 
     if (MI.getOpcode() != TargetOpcode::G_ZEXT)
       return false;
     MachineInstr *DefMI = MRI.getVRegDef(MI.getOperand(1).getReg());
     if (DefMI->getOpcode() == TargetOpcode::G_TRUNC) {
+      unsigned DstReg = MI.getOperand(0).getReg();
+      LLT DstTy = MRI.getType(DstReg);
+      if (isInstUnsupported(TargetOpcode::G_AND, DstTy) ||
+          isInstUnsupported(TargetOpcode::G_CONSTANT, DstTy))
+        return false;
       DEBUG(dbgs() << ".. Combine MI: " << MI;);
       Builder.setInstr(MI);
-      unsigned DstReg = MI.getOperand(0).getReg();
       unsigned ZExtSrc = MI.getOperand(1).getReg();
       LLT ZExtSrcTy = MRI.getType(ZExtSrc);
-      LLT DstTy = MRI.getType(DstReg);
       APInt Mask = APInt::getAllOnesValue(ZExtSrcTy.getSizeInBits());
       auto MaskCstMIB = Builder.buildConstant(DstTy, Mask.getZExtValue());
       unsigned TruncSrc = DefMI->getOperand(1).getReg();
       // We get a copy/trunc/extend depending on the sizes
       auto SrcCopyOrTrunc = Builder.buildAnyExtOrTrunc(DstTy, TruncSrc);
       Builder.buildAnd(DstReg, SrcCopyOrTrunc, MaskCstMIB);
       markInstAndDefDead(MI, *DefMI, DeadInsts);
       return true;
     }
     return false;
   }
 
   bool tryCombineSExt(MachineInstr &MI,
                       SmallVectorImpl<MachineInstr *> &DeadInsts) {
 
     if (MI.getOpcode() != TargetOpcode::G_SEXT)
       return false;
     MachineInstr *DefMI = MRI.getVRegDef(MI.getOperand(1).getReg());
     if (DefMI->getOpcode() == TargetOpcode::G_TRUNC) {
-      DEBUG(dbgs() << ".. Combine MI: " << MI;);
-      Builder.setInstr(MI);
       unsigned DstReg = MI.getOperand(0).getReg();
       LLT DstTy = MRI.getType(DstReg);
+      if (isInstUnsupported(TargetOpcode::G_SHL, DstTy) ||
+          isInstUnsupported(TargetOpcode::G_ASHR, DstTy))
+        return false;
+      DEBUG(dbgs() << ".. Combine MI: " << MI;);
+      Builder.setInstr(MI);
       unsigned SExtSrc = MI.getOperand(1).getReg();
       LLT SExtSrcTy = MRI.getType(SExtSrc);
       unsigned SizeDiff = DstTy.getSizeInBits() - SExtSrcTy.getSizeInBits();
       auto SizeDiffMIB = Builder.buildConstant(DstTy, SizeDiff);
       unsigned TruncSrcReg = DefMI->getOperand(1).getReg();
       // We get a copy/trunc/extend depending on the sizes
       auto SrcCopyExtOrTrunc = Builder.buildAnyExtOrTrunc(DstTy, TruncSrcReg);
       auto ShlMIB = Builder.buildInstr(TargetOpcode::G_SHL, DstTy,
                                        SrcCopyExtOrTrunc, SizeDiffMIB);
       Builder.buildInstr(TargetOpcode::G_ASHR, DstReg, ShlMIB, SizeDiffMIB);
       markInstAndDefDead(MI, *DefMI, DeadInsts);
       return true;
private:
   /// Mark MI as dead. If a def of one of MI's operands, DefMI, would also be
   /// dead due to MI being killed, then mark DefMI as dead too.
   void markInstAndDefDead(MachineInstr &MI, MachineInstr &DefMI,
                           SmallVectorImpl<MachineInstr *> &DeadInsts) {
     DeadInsts.push_back(&MI);
     if (MRI.hasOneUse(DefMI.getOperand(0).getReg()))
       DeadInsts.push_back(&DefMI);
   }
+  /// Checks if the target legalizer info has specified anything about the
+  /// instruction, or if unsupported.
+  bool isInstUnsupported(unsigned Opcode, const LLT &DstTy) const {
+    auto Action = LI.getAction({Opcode, 0, DstTy});
+    return Action.first == LegalizerInfo::LegalizeAction::Unsupported ||
+           Action.first == LegalizerInfo::LegalizeAction::NotFound;
+  }
 };
 
 } // namespace llvm

llvm/trunk/include/llvm/CodeGen/GlobalISel/LegalizerHelper.h

public:
   /// involved and ignoring the added elements later.
   LegalizeResult moreElementsVector(MachineInstr &MI, unsigned TypeIdx,
                                     LLT WideTy);
 
   /// Expose MIRBuilder so clients can set their own RecordInsertInstruction
   /// functions
   MachineIRBuilder MIRBuilder;
 
+  /// Expose LegalizerInfo so the clients can re-use.
+  const LegalizerInfo &getLegalizerInfo() const { return LI; }
+
 private:
 
   /// Helper function to split a wide generic register into bitwise blocks with
   /// the given Type (which implies the number of blocks needed). The generic
   /// registers created are appended to Ops, starting at bit 0 of Reg.
   void extractParts(unsigned Reg, LLT Ty, int NumParts,
                     SmallVectorImpl<unsigned> &Ops);
 

llvm/trunk/lib/CodeGen/GlobalISel/Legalizer.cpp

for (auto MI = MBB.begin(); MI != MBB.end(); MI = NextMI) {
         // instructions with generic types. Don't record them
         if (isPreISelGenericOpcode(MI->getOpcode())) {
           ++NumNewInsns;
           WorkList.insert(MI);
           CombineList.insert(MI);
         }
       });
       WorkList.insert(&*MI);
-      LegalizerCombiner C(Helper.MIRBuilder, MF.getRegInfo());
+      LegalizerCombiner C(Helper.MIRBuilder, MF.getRegInfo(),
+                          Helper.getLegalizerInfo());
       bool Changed = false;
       LegalizerHelper::LegalizeResult Res;
       do {
         assert(!WorkList.empty() && "Expecting illegal ops");
         while (!WorkList.empty()) {
           NumNewInsns = 0;
           MachineInstr *CurrInst = WorkList.pop_back_val();
           Res = Helper.legalizeInstrStep(*CurrInst);
#endif
       } while (!WorkList.empty());
 
       Helper.MIRBuilder.stopRecordingInsertions();
     }
   }
 
   MachineRegisterInfo &MRI = MF.getRegInfo();
   MachineIRBuilder MIRBuilder(MF);
-  LegalizerCombiner C(MIRBuilder, MRI);
+  LegalizerCombiner C(MIRBuilder, MRI, Helper.getLegalizerInfo());
   for (auto &MBB : MF) {
     for (auto MI = MBB.begin(); MI != MBB.end(); MI = NextMI) {
       // Get the next Instruction before we try to legalize, because there's a
       // good chance MI will be deleted.
       // TOOD: Perhaps move this to a combiner pass later?.
       NextMI = std::next(MI);
       SmallVector<MachineInstr *, 4> DeadInsts;
       Changed |= C.tryCombineMerges(*MI, DeadInsts);
       for (auto *DeadMI : DeadInsts)
         DeadMI->eraseFromParent();
     }
   }
 
   return Changed;
 }