GlobalISel/Utils: Refactor integer/float constant match functions

[Petar.Avramovic]

Summary

Rework getConstantstVRegValWithLookThrough in order to make it clear if we
are matching integer/float constant only or any constant(default).
Add helper functions that get DefVReg and APInt/APFloat from constant instr
getIConstantVRegValWithLookThrough: integer constant, only G_CONSTANT
getFConstantVRegValWithLookThrough: float constant, only G_FCONSTANT
getAnyConstantVRegValWithLookThrough: either G_CONSTANT or G_FCONSTANT

Rename getConstantVRegVal and getConstantVRegSExtVal to getIConstantVRegVal
and getIConstantVRegSExtVal. These now only match G_CONSTANT as described
in comment.

Relevant matchers now return both DefVReg and APInt/APFloat.

Replace existing uses of getConstantstVRegValWithLookThrough and
getConstantVRegVal with new helper functions. Any constant match is
only required in:
ConstantFoldBinOp: for constant argument that was bit-cast of float to int
getAArch64VectorSplat: AArch64::G_DUP operands can be any constant
amdgpu select for G_BUILD_VECTOR_TRUNC: operands can be any constant

In other places use integer only constant match.

[paquette]

I'm not sure if I like having two toggles here because now people could set HandleFConstants == false and HandleIConstants == false. :/

I think that it would be nice if we could expose helper functions like so:

• getIConstantVRegValWithLookThrough => integer constant case only
• getFConstantVRegValWithLookThrough => fp constant case only
• getAnyConstantVRegValWithLookThrough => any constant

And then have getConstantVRegValWithLookThrough be inaccessible outside of Utils.

This would be a bit more invasive of a change, but I think it would be a readability win overall.

[Petar.Avramovic]

Try to have more strict match functions. Match G or G_F Constant only when there is a reason for it.

[Petar.Avramovic]

llvm/lib/CodeGen/GlobalISel/Utils.cpp

373–374

I thought to make this public. There are many opcode checks like this, most often in asserts.
Do you find it useful?

[paquette]

What is the motivation behind adding G to the function names? Given that GlobalISel uses G_* as a prefix for all opcode names, it seems kind of redundant to me.

llvm/lib/CodeGen/GlobalISel/Utils.cpp

373–374

I don't know if it really improves on the existing opcode checks all too much.

(This would also probably need a check for nullptr or an assert?)

382

Check for nullptr or assert?

[arsenm]

Do we even really need G_FCONSTANT? I've always thought having a separate node/opcode for these was just pointless complexity that can just as well be tracked with an APInt

llvm/lib/CodeGen/GlobalISel/Utils.cpp

373–374

I think a function to check a single case is just noise

373–374

Or just use a reference, shouldn't handle null

[paquette]

I think it would be easier to think about removing it if we had FP types.

I think it helps with handling things like selects/phis in AArch64 right now.

[arsenm]

llvm/include/llvm/CodeGen/GlobalISel/Utils.h

190

This name is way less readable

[Petar.Avramovic]

What is the motivation behind adding G to the function names?

GlobalISel does not mention IConstant it is just G_CONSTANT but has FConstant (G_FCONSTANT). It thought that GCst in name associates better with G_CONSTANT instruction (which we know to be integer constant). Matchers use this naming style m_GAdd - integer add vs m_GFAdd - float.
Only Constant is ambiguous and is good name for generic helper function.

llvm/lib/CodeGen/GlobalISel/Utils.cpp

316

This guards nullptr from entering into isGConstant and other opcodes checks.

342

No nullptrs here either.

[Petar.Avramovic]

Ping.

[Petar.Avramovic]

Ping.

[Petar.Avramovic]

Rebase and ping.

[Petar.Avramovic]

Rebase and ping.

[Petar.Avramovic]

Rebase and ping.

[Petar.Avramovic]

Rebase and ping.
Brief summary:
Add support for g_fconstant matching and option to return APFloat.
Rename getConstantVRegValWithLookThrough into get{I|F|Any}ConstantVRegValWithLookThrough. Remove HandleFConstant argument, use function with appropriate name instead.
Add matchers for Reg and APInt/Float: g_constant m_GCst(&ValueAndVReg) and g_fconstant m_GFCst(&FPValueAndVReg)

[foad]

@paquette do you have further comments on this? We have various (approved) patches waiting on this one.

[paquette]

LGTM

llvm/include/llvm/CodeGen/GlobalISel/MIPatternMatch.h

if (auto MaybeCst = getConstantVRegSExtVal(Reg, MRI)) {
       return true;
     }
     return false;
   }
 };
 
 inline ConstantMatch m_ICst(int64_t &Cst) { return ConstantMatch(Cst); }
 
-struct ICstRegMatch {
+template <bool HandleFConstants, bool HandleIConstants> struct CstRegMatch {
   Register &CR;
-  ICstRegMatch(Register &C) : CR(C) {}
+  CstRegMatch(Register &C) : CR(C) {}
   bool match(const MachineRegisterInfo &MRI, Register Reg) {
     if (auto MaybeCst = getConstantVRegValWithLookThrough(
-            Reg, MRI, /*LookThroughInstrs*/ true,
-            /*HandleFConstants*/ false)) {
+            Reg, MRI, /*LookThroughInstrs*/ true, HandleFConstants,
+            HandleIConstants)) {
       CR = MaybeCst->VReg;
       return true;
     }
     return false;
   }
 };
 
+struct ICstRegMatch : CstRegMatch<false, true> {
+  ICstRegMatch(Register &C) : CstRegMatch(C) {}
+};
 inline ICstRegMatch m_ICst(Register &Reg) { return ICstRegMatch(Reg); }
 
+struct FCstRegMatch : CstRegMatch<true, false> {
+  FCstRegMatch(Register &C) : CstRegMatch(C) {}
+};
+inline FCstRegMatch m_FCst(Register &Reg) { return FCstRegMatch(Reg); }
+
 /// Matcher for a specific constant value.
 struct SpecificConstantMatch {
   int64_t RequestedVal;
   SpecificConstantMatch(int64_t RequestedVal) : RequestedVal(RequestedVal) {}
   bool match(const MachineRegisterInfo &MRI, Register Reg) {
     int64_t MatchedVal;
     return mi_match(Reg, MRI, m_ICst(MatchedVal)) && MatchedVal == RequestedVal;
   }

llvm/include/llvm/CodeGen/GlobalISel/Utils.h

 };
 /// If \p VReg is defined by a statically evaluable chain of
 /// instructions rooted on a G_F/CONSTANT (\p LookThroughInstrs == true)
 /// and that constant fits in int64_t, returns its value as well as the
 /// virtual register defined by this G_F/CONSTANT.
 /// When \p LookThroughInstrs == false this function behaves like
 /// getConstantVRegVal.
 /// When \p HandleFConstants == false the function bails on G_FCONSTANTs.
+/// When \p HandleIConstants == false the function bails on G_CONSTANTs.
 /// When \p LookThroughAnyExt == true the function treats G_ANYEXT same as
 /// G_SEXT.
 Optional<ValueAndVReg>
 getConstantVRegValWithLookThrough(Register VReg, const MachineRegisterInfo &MRI,
                                   bool LookThroughInstrs = true,
                                   bool HandleFConstants = true,
+                                  bool HandleIConstants = true,
                                   bool LookThroughAnyExt = false);
 const ConstantInt *getConstantIntVRegVal(Register VReg,
                                          const MachineRegisterInfo &MRI);
 const ConstantFP* getConstantFPVRegVal(Register VReg,
                                        const MachineRegisterInfo &MRI);
 
 /// See if Reg is defined by an single def instruction that is
 /// Opcode. Also try to do trivial folding if it's a COPY with

llvm/lib/CodeGen/GlobalISel/Utils.cpp

Optional<int64_t> llvm::getConstantVRegSExtVal(Register VReg,
   Optional<APInt> Val = getConstantVRegVal(VReg, MRI);
   if (Val && Val->getBitWidth() <= 64)
     return Val->getSExtValue();
   return None;
 }
 
 Optional<ValueAndVReg> llvm::getConstantVRegValWithLookThrough(
     Register VReg, const MachineRegisterInfo &MRI, bool LookThroughInstrs,
-    bool HandleFConstant, bool LookThroughAnyExt) {
+    bool HandleFConstant, bool HandleIConstant, bool LookThroughAnyExt) {
   SmallVector<std::pair<unsigned, unsigned>, 4> SeenOpcodes;
   MachineInstr *MI;
-  auto IsConstantOpcode = [HandleFConstant](unsigned Opcode) {
-    return Opcode == TargetOpcode::G_CONSTANT ||
+  auto IsConstantOpcode = [HandleFConstant, HandleIConstant](unsigned Opcode) {
+    return (HandleIConstant && Opcode == TargetOpcode::G_CONSTANT) ||
            (HandleFConstant && Opcode == TargetOpcode::G_FCONSTANT);
   };
   auto GetImmediateValue = [HandleFConstant,
                             &MRI](const MachineInstr &MI) -> Optional<APInt> {
     const MachineOperand &CstVal = MI.getOperand(1);
     if (!CstVal.isImm() && !CstVal.isCImm() &&
         (!HandleFConstant || !CstVal.isFPImm()))
       return None;
     if (!CstVal.isFPImm()) {
       unsigned BitWidth =
           MRI.getType(MI.getOperand(0).getReg()).getSizeInBits();
       APInt Val = CstVal.isImm() ? APInt(BitWidth, CstVal.getImm())
                                  : CstVal.getCImm()->getValue();
       assert(Val.getBitWidth() == BitWidth &&
              "Value bitwidth doesn't match definition type");
       return Val;
     }
     return CstVal.getFPImm()->getValueAPF().bitcastToAPInt();
   };
   while ((MI = MRI.getVRegDef(VReg)) && !IsConstantOpcode(MI->getOpcode()) &&
          LookThroughInstrs) {
     switch (MI->getOpcode()) {
     case TargetOpcode::G_ANYEXT:
       if (!LookThroughAnyExt)
         return None;
       LLVM_FALLTHROUGH;
     case TargetOpcode::G_TRUNC:
     case TargetOpcode::G_SEXT:
     case TargetOpcode::G_ZEXT:
case TargetOpcode::COPY:
       break;
     case TargetOpcode::G_INTTOPTR:
       VReg = MI->getOperand(1).getReg();
       break;
     default:
       return None;
     }
   }
   if (!MI || !IsConstantOpcode(MI->getOpcode()))
     return None;
 
   Optional<APInt> MaybeVal = GetImmediateValue(*MI);
   if (!MaybeVal)
     return None;
   APInt &Val = *MaybeVal;
   while (!SeenOpcodes.empty()) {
     std::pair<unsigned, unsigned> OpcodeAndSize = SeenOpcodes.pop_back_val();
Optional<ValueAndVReg> llvm::getConstantVRegValWithLookThrough(
   return ValueAndVReg{Val, VReg};
 }
 
 const ConstantInt *llvm::getConstantIntVRegVal(Register VReg,
                                                const MachineRegisterInfo &MRI) {
   MachineInstr *MI = MRI.getVRegDef(VReg);
   if (MI->getOpcode() != TargetOpcode::G_CONSTANT)
     return nullptr;
   return MI->getOperand(1).getCImm();
 }
 
 const ConstantFP *
 llvm::getConstantFPVRegVal(Register VReg, const MachineRegisterInfo &MRI) {
   MachineInstr *MI = MRI.getVRegDef(VReg);
   if (TargetOpcode::G_FCONSTANT != MI->getOpcode())
     return nullptr;
   return MI->getOperand(1).getFPImm();
 }
 
 Optional<DefinitionAndSourceRegister>
 llvm::getDefSrcRegIgnoringCopies(Register Reg, const MachineRegisterInfo &MRI) {
   Register DefSrcReg = Reg;
   auto *DefMI = MRI.getVRegDef(Reg);
   auto DstTy = MRI.getType(DefMI->getOperand(0).getReg());
   if (!DstTy.isValid())
     return None;

llvm/lib/Target/AMDGPU/AMDGPUInstructionSelector.cpp

bool AMDGPUInstructionSelector::selectG_BUILD_VECTOR_TRUNC(
   Register Src1 = MI.getOperand(2).getReg();
   if (MRI->getType(Src0) != S32)
     return false;
 
   const DebugLoc &DL = MI.getDebugLoc();
   MachineBasicBlock *BB = MI.getParent();
 
   auto ConstSrc1 =
-      getConstantVRegValWithLookThrough(Src1, *MRI, true, true, true);
+      getConstantVRegValWithLookThrough(Src1, *MRI, true, true, true, true);
   if (ConstSrc1) {
     auto ConstSrc0 =
-        getConstantVRegValWithLookThrough(Src0, *MRI, true, true, true);
+        getConstantVRegValWithLookThrough(Src0, *MRI, true, true, true, true);
     if (ConstSrc0) {
       const int64_t K0 = ConstSrc0->Value.getSExtValue();
       const int64_t K1 = ConstSrc1->Value.getSExtValue();
       uint32_t Lo16 = static_cast<uint32_t>(K0) & 0xffff;
       uint32_t Hi16 = static_cast<uint32_t>(K1) & 0xffff;
 
       BuildMI(*BB, &MI, DL, TII.get(AMDGPU::S_MOV_B32), Dst)
         .addImm(Lo16 | (Hi16 << 16));

llvm/unittests/CodeGen/GlobalISel/PatternMatchTest.cpp

if (!TM)
     return;
   auto AllOnes = B.buildConstant(LLT::scalar(64), -1);
   EXPECT_TRUE(mi_match(AllOnes.getReg(0), *MRI, m_AllOnesInt()));
 
   auto FortyTwo = B.buildConstant(LLT::scalar(64), 42);
   EXPECT_FALSE(mi_match(FortyTwo.getReg(0), *MRI, m_AllOnesInt()));
 }
 
+TEST_F(AArch64GISelMITest, MatchFPOrIntConstExplicitly) {
+  setUp();
+  if (!TM)
+    return;
+
+  Register IntOne = B.buildConstant(LLT::scalar(64), 1).getReg(0);
+  Register FPOne = B.buildFConstant(LLT::scalar(64), 1.0).getReg(0);
+  Register Reg;
+
+  EXPECT_TRUE(mi_match(IntOne, *MRI, m_ICst(Reg)));
+  EXPECT_EQ(IntOne, Reg);
+  EXPECT_FALSE(mi_match(IntOne, *MRI, m_FCst(Reg)));
+
+  EXPECT_FALSE(mi_match(FPOne, *MRI, m_ICst(Reg)));
+  EXPECT_TRUE(mi_match(FPOne, *MRI, m_FCst(Reg)));
+  EXPECT_EQ(FPOne, Reg);
+
+  const bool HandleFConstants = true;
+  const bool HandleIConstants = true;
+  // Match any constant
+  EXPECT_TRUE(mi_match(IntOne, *MRI,
+                       CstRegMatch<HandleFConstants, HandleIConstants>(Reg)));
+  EXPECT_EQ(IntOne, Reg);
+
+  EXPECT_TRUE(mi_match(FPOne, *MRI,
+                       CstRegMatch<HandleFConstants, HandleIConstants>(Reg)));
+  EXPECT_EQ(FPOne, Reg);
+}
+
 TEST_F(AArch64GISelMITest, MatchNeg) {
   setUp();
   if (!TM)
     return;
 
   LLT s64 = LLT::scalar(64);
   auto Zero = B.buildConstant(LLT::scalar(64), 0);
   auto NegInst = B.buildSub(s64, Zero, Copies[0]);