Add basic functionality for assignment of ints. This creates a lot of core infrastructure in which to add, with little effort, quite a bit more to mips fast-isel

[rkotler]

I messed up this arc call. Hopefully this adds the rest of the files.

Updating D3527: Add basic functionality for assignment of ints.

This creates a lot of core infrastructure in which to add, with little
effort, quite a bit more to mips fast-isel

[dsanders]

I've added a few comments inline.

One high-level suggestion I'd like to make is that I think it would be good to flesh out the materialization of constants before you go much further on things that depend on materializing constants. I'm thinking along the lines of a set of tests similar to nullvoid.ll with 'return i8 1', 'return i32 65536', etc.

lib/Target/Mips/MipsFastISel.cpp

20

plus some?

29

This should probably be an APInt or at least an int64_t

96–100

It looks like you are accounting for future changes here. When would this be different from isTypeLegal()?

103–106

It would be better to make this a test to allow things you are able to handle than a test to exclude things you can't.

For example, it seems unlikely that ConstantArray or FPMathOperator are an acceptable values but these if-statements will currently accept them.

117–118

In isTypeLegal() you checked for MVT::Other as well as isSimple(). Is that not necessary here?

133–134

It would be good to have a fixme comment explaining that this is temporary

165–167

I'd invert this if-statement to:

if (EmitStore(...))
  return true;
return false;

208

In what way is it broken?

210–211

Is the IsThreadLocal variable necessary? It looks like it would be more readable in the if-statement condition

[rkotler]

I agree with your high level comment regarding finishing materialize. I will work on more incremental changes to finish that. I'm essentially copying what other ports do, and modifying it for Mips specifics.

I'm cut and pasting some code from other ports in other areas too and I agree that at time it's awkward because I'm taking out pieces and those that remain can be structured better.

I'll take out things as you suggested that are not being used now and clean things.

I will add it back in as necessary in order to keep the code looking nice.

[rkotler]

Essentially, all these fast-isel ports look the same so I know, at least for this first version, what most of these functions will eventually look like.

I'm trying not to allow things to pass through fast-isel that we can't handle yet; but it's not easy to always know.
The existing ports are the best way barring spending months reading the supporting code.

lib/Target/Mips/MipsFastISel.cpp

96–100

This is copied from other ports. They allow types that can be simply zero or sign extended like i1, i8 and i16

I will add a comment to that effect.

103–106

This is kind of a catch 22 in terms of what I am doing here.
I'm stripping down and porting and existing port.

I'm making sure that I don't accept cases that I don't intend to by mimicking the behavior of other ports.

[rkotler]

I will try and do most of what you want except in cases where it will take a huge amount of time because the code will morph back to how it looks now when the additional features are added.

[rkotler]

Updating D3527: Add basic functionality for assignment of ints.

This creates a lot of core infrastructure in which to add, with little
effort, quite a bit more to mips fast-isel

[dsanders]

With the following changes it will LGTM:

• Address::Offset changed to an APInt or at least an int64_t
• The various FIXME comments added. Particularly the ones that point out thing that are present in other ports but not implemented yet. They will make it obvious that some of the odd-looking style only looks odd because of the missing pieces.
• The ARM-specific comment about FastISel TLS changed to be accurate for MIPS.

lib/Target/Mips/MipsFastISel.cpp

103–106

Ok, having looked at the other ports I see what you are doing.

A few fixme comments mentioning the missing pieces would be useful.

165–167

I still think this if-statement ought to be inverted but I see all the other ports are using the same style. Up to you if you invert it or not.

208

Having looked at the other ports, this comment seems to be ARM specific. Please change it to something like 'FastISel TLS support is not implemented, punt to SelectionDAG'.

[rkotler]

Updating D3527: Add basic functionality for assignment of ints.

This creates a lot of core infrastructure in which to add, with little
effort, quite a bit more to mips fast-isel

lib/Target/Mips/MipsFastISel.cpp

 //===-- MipsastISel.cpp - Mips FastISel implementation
 //---------------------===//
 
 #include "llvm/CodeGen/FunctionLoweringInfo.h"
 #include "llvm/CodeGen/FastISel.h"
-
 #include "llvm/CodeGen/MachineInstrBuilder.h"
+#include "llvm/IR/GlobalAlias.h"
+#include "llvm/IR/GlobalVariable.h"
 #include "llvm/Target/TargetInstrInfo.h"
 #include "llvm/Target/TargetLibraryInfo.h"
+#include "MipsRegisterInfo.h"
 #include "MipsISelLowering.h"
 #include "MipsMachineFunction.h"
 #include "MipsSubtarget.h"
 
 using namespace llvm;
 
 namespace {
 
+// All possible address modes.
+typedef struct Address {
+  enum { RegBase, FrameIndexBase } BaseType;
+
+  union {
+    unsigned Reg;
+    int FI;
+  } Base;
+
+  int64_t Offset;
+
+  // Innocuous defaults for our address.
+  Address() : BaseType(RegBase), Offset(0) { Base.Reg = 0; }
+} Address;
+
 class MipsFastISel final : public FastISel {
 
   /// Subtarget - Keep a pointer to the MipsSubtarget around so that we can
   /// make the right decision when generating code for different targets.
   const MipsSubtarget *Subtarget;
   Module &M;
   const TargetMachine &TM;
   const TargetInstrInfo &TII;
explicit MipsFastISel(FunctionLoweringInfo &funcInfo,
     Subtarget = &TM.getSubtarget<MipsSubtarget>();
     MFI = funcInfo.MF->getInfo<MipsFunctionInfo>();
     Context = &funcInfo.Fn->getContext();
     TargetSupported = ((Subtarget->getRelocationModel() == Reloc::PIC_) &&
                        (Subtarget->hasMips32r2() && (Subtarget->isABI_O32())));
   }
 
   bool TargetSelectInstruction(const Instruction *I) override;
+  unsigned TargetMaterializeConstant(const Constant *C) override;
+
+  bool ComputeAddress(const Value *Obj, Address &Addr);
 
+private:
+  bool EmitStore(MVT VT, unsigned SrcReg, Address &Addr,
+                 unsigned Alignment = 0);
   bool SelectRet(const Instruction *I);
+  bool SelectStore(const Instruction *I);
+
+  bool isTypeLegal(Type *Ty, MVT &VT);
+  bool isLoadTypeLegal(Type *Ty, MVT &VT);
+
+  unsigned MaterializeFP(const ConstantFP *CFP, MVT VT);
+  unsigned MaterializeGV(const GlobalValue *GV, MVT VT);
+  unsigned MaterializeInt(const Constant *C, MVT VT);
 };
 
+bool MipsFastISel::isTypeLegal(Type *Ty, MVT &VT) {
+  EVT evt = TLI.getValueType(Ty, true);
+  // Only handle simple types.
+  if (evt == MVT::Other || !evt.isSimple())
+    return false;
+  VT = evt.getSimpleVT();
+
+  // Handle all legal types, i.e. a register that will directly hold this
+  // value.
+  return TLI.isTypeLegal(VT);
+}
+
+bool MipsFastISel::isLoadTypeLegal(Type *Ty, MVT &VT) {
+  if (isTypeLegal(Ty, VT))
+    return true;
+  // We will extend this in a later patch:
+  //   If this is a type than can be sign or zero-extended to a basic operation
+  //   go ahead and accept it now.
+  return false;
+}
+
+bool MipsFastISel::ComputeAddress(const Value *Obj, Address &Addr) {
+  // This construct looks a big awkward but it is how other ports handle this
+  // and as this function is more fully completed, these cases which
+  // return false will have additional code in them.
+  //
+  if (isa<Instruction>(Obj))
+    return false;
+  else if (isa<ConstantExpr>(Obj))
+    return false;
+  Addr.Base.Reg = getRegForValue(Obj);
+  return Addr.Base.Reg != 0;
+}
+
+// Materialize a constant into a register, and return the register
+// number (or zero if we failed to handle it).
+unsigned MipsFastISel::TargetMaterializeConstant(const Constant *C) {
+  EVT CEVT = TLI.getValueType(C->getType(), true);
+
+  // Only handle simple types.
+  if (!CEVT.isSimple())
+    return 0;
+  MVT VT = CEVT.getSimpleVT();
+
+  if (const ConstantFP *CFP = dyn_cast<ConstantFP>(C))
+    return MaterializeFP(CFP, VT);
+  else if (const GlobalValue *GV = dyn_cast<GlobalValue>(C))
+    return MaterializeGV(GV, VT);
+  else if (isa<ConstantInt>(C))
+    return MaterializeInt(C, VT);
+
+  return 0;
+}
+
+bool MipsFastISel::EmitStore(MVT VT, unsigned SrcReg, Address &Addr,
+                             unsigned Alignment) {
+  //
+  // more cases will be handled here in following patches.
+  //
+  if (VT != MVT::i32)
+    return false;
+  BuildMI(*FuncInfo.MBB, FuncInfo.InsertPt, DbgLoc, TII.get(Mips::SW))
+      .addReg(SrcReg)
+      .addReg(Addr.Base.Reg)
+      .addImm(Addr.Offset);
+  return true;
+}
+
+bool MipsFastISel::SelectStore(const Instruction *I) {
+  Value *Op0 = I->getOperand(0);
+  unsigned SrcReg = 0;
+
+  // Atomic stores need special handling.
+  if (cast<StoreInst>(I)->isAtomic())
+    return false;
+
+  // Verify we have a legal type before going any further.
+  MVT VT;
+  if (!isLoadTypeLegal(I->getOperand(0)->getType(), VT))
+    return false;
+
+  // Get the value to be stored into a register.
+  SrcReg = getRegForValue(Op0);
+  if (SrcReg == 0)
+    return false;
+
+  // See if we can handle this address.
+  Address Addr;
+  if (!ComputeAddress(I->getOperand(1), Addr))
+    return false;
+
+  if (!EmitStore(VT, SrcReg, Addr, cast<StoreInst>(I)->getAlignment()))
+    return false;
+  return true;
+}
+
 bool MipsFastISel::SelectRet(const Instruction *I) {
   const ReturnInst *Ret = cast<ReturnInst>(I);
 
   if (!FuncInfo.CanLowerReturn)
     return false;
   if (Ret->getNumOperands() > 0) {
     return false;
   }
   unsigned RetOpc = Mips::RetRA;
   BuildMI(*FuncInfo.MBB, FuncInfo.InsertPt, DbgLoc, TII.get(RetOpc));
   return true;
 }
 
 bool MipsFastISel::TargetSelectInstruction(const Instruction *I) {
   if (!TargetSupported)
     return false;
   switch (I->getOpcode()) {
   default:
     break;
+  case Instruction::Store:
+    return SelectStore(I);
   case Instruction::Ret:
     return SelectRet(I);
   }
   return false;
 }
 }
 
+unsigned MipsFastISel::MaterializeFP(const ConstantFP *CFP, MVT VT) {
+  return 0;
+}
+
+unsigned MipsFastISel::MaterializeGV(const GlobalValue *GV, MVT VT) {
+  // For now 32-bit only.
+  if (VT != MVT::i32)
+    return 0;
+  const TargetRegisterClass *RC = &Mips::GPR32RegClass;
+  unsigned DestReg = createResultReg(RC);
+  const GlobalVariable *GVar = dyn_cast<GlobalVariable>(GV);
+  bool IsThreadLocal = GVar && GVar->isThreadLocal();
+  // TLS not supported at this time.
+  if (IsThreadLocal)
+    return 0;
+  BuildMI(*FuncInfo.MBB, FuncInfo.InsertPt, DbgLoc, TII.get(Mips::LW), DestReg)
+      .addReg(MFI->getGlobalBaseReg())
+      .addGlobalAddress(GV, 0, MipsII::MO_GOT);
+  return DestReg;
+}
+unsigned MipsFastISel::MaterializeInt(const Constant *C, MVT VT) {
+  if (VT != MVT::i32)
+    return 0;
+  const TargetRegisterClass *RC = &Mips::GPR32RegClass;
+  // If the constant is in range, use a load-immediate.
+  const ConstantInt *CI = cast<ConstantInt>(C);
+  if (isInt<16>(CI->getSExtValue())) {
+    unsigned Opc = Mips::ADDiu;
+    unsigned ImmReg = createResultReg(RC);
+    BuildMI(*FuncInfo.MBB, FuncInfo.InsertPt, DbgLoc, TII.get(Opc), ImmReg)
+        .addReg(Mips::ZERO)
+        .addImm(CI->getSExtValue());
+    return ImmReg;
+  }
+  return 0;
+}
+
 namespace llvm {
 FastISel *Mips::createFastISel(FunctionLoweringInfo &funcInfo,
                                const TargetLibraryInfo *libInfo) {
   return new MipsFastISel(funcInfo, libInfo);
 }
 }

test/CodeGen/Mips/Fast-ISel/simplestore.ll

+; RUN: llc -march=mipsel -relocation-model=pic -O0 -mips-fast-isel -fast-isel-abort -mcpu=mips32r2 \
+; RUN:     < %s | FileCheck %s
+
+@abcd = external global i32
+
+; Function Attrs: nounwind
+define void @foo()  {
+entry:
+  store i32 12345, i32* @abcd, align 4
+; CHECK:  addiu $[[REG1:[0-9]+]], $zero, 12345
+; CHECK:  lw  $[[REG2:[0-9]+]], %got(abcd)(${{[0-9]+}})
+; CHECK:  sw  $[[REG1]], 0($[[REG2]])
+  ret void
+}
+