fix materialization of one bit constants and global values which are accessed through a base GOT entry.

[rkotler]

Summary

get tip of tree mips fast-isel to pass test-suite

Two bugs were fixed:

1. one bit booleans were treated as 1 bit signed integers and so the literal '1' could become sign extended.
2. mips uses got for pic but in certain cases, as with string constants for example, many items can be referenced from the same got entry and this case was not handled properly.

[dsanders]

Hi Reed,

You'll need to correct the title and commit message since they're the concatenation of the commits on your branch at the moment. You can use the 'Edit Revision' link on the top right of the web interface.

lib/Target/Mips/MipsFastISel.cpp

19–23

It ought to be a given that the test-suite will pass (bugs aside) so I'm really not keen on a 'make it pass' option. Also, there's already a -mips-fast-isel option that keeps the fast isel from being used by default until it's ready. -mips-fast-isel-pass-test-suite seems to be serving the same purpose.

[rkotler]

Make test-suite pass (build bots green for fast-isel mips -O0 and -O2

[rkotler]

• remove blank from test case

[rkotler]

• take out uneeded include

[dsanders]

Thanks. That's much better.

LGTM with three nits. Two nits and the optional change are described inline. The other nit is that you don't need to say 'tip of tree' in the title/summary since it's a given that the commit affects the tip of the tree and that it's better to focus the commit message (particularly the title/subject line) on what the commit changes rather than what it achieves (e.g. "Fix materialization of i1 constants" rather than "Get test-suite to pass"). There's also an optional change which you may or may not choose to do.

As we discussed off-review, it would normally be better to do one bugfix per patch but the urgency of the MIPS32r6/MIPS64r6 work has meant that you haven't been able to get any reviews for a long time. I'm therefore willing to compromise on this a bit.

lib/Target/Mips/MipsFastISel.cpp

358

Nit: Could you use something other than 'DestReg_' here? It's a bit confusing when there's non-trivial code that uses 'DestReg' and 'DestReg_'.

360

This change is up to you since the current aim is to support O32. You could trivially support N32/N64 here. You would only need to switch MO_ABS_LO for MO_GOT_OFST.

365

blank line needed between functions

[rkotler]

• nits
• Merge branch 'master' into reed-bug-02

lib/Target/Mips/MipsFastISel.cpp

 #include "MipsRegisterInfo.h"
 #include "MipsISelLowering.h"
 #include "MipsMachineFunction.h"
 #include "MipsSubtarget.h"
 #include "MipsTargetMachine.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;
 
private:
   }
 
   MachineInstrBuilder EmitInstStore(unsigned Opc, unsigned SrcReg,
                                     unsigned MemReg, int64_t MemOffset) {
     return EmitInst(Opc).addReg(SrcReg).addReg(MemReg).addImm(MemOffset);
   }
 
   MachineInstrBuilder EmitInstLoad(unsigned Opc, unsigned DstReg,
-                                      unsigned MemReg, int64_t MemOffset) {
     return EmitInst(Opc, DstReg).addReg(MemReg).addImm(MemOffset);
   }
 
 #include "MipsGenFastISel.inc"
 };
 
 bool MipsFastISel::isTypeLegal(Type *Ty, MVT &VT) {
   EVT evt = TLI.getValueType(Ty, true);
unsigned MipsFastISel::MaterializeGV(const GlobalValue *GV, MVT VT) {
   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;
   EmitInst(Mips::LW, DestReg).addReg(MFI->getGlobalBaseReg()).addGlobalAddress(
       GV, 0, MipsII::MO_GOT);
+  if ((GV->hasInternalLinkage() ||
+       (GV->hasLocalLinkage() && !isa<Function>(GV)))) {
+    unsigned DestReg_ = createResultReg(RC);
+    EmitInst(Mips::ADDiu, DestReg_).addReg(DestReg).addGlobalAddress(
+        GV, 0, MipsII::MO_ABS_LO);
+    DestReg = DestReg_;
+  }
   return DestReg;
 }
 unsigned MipsFastISel::MaterializeInt(const Constant *C, MVT VT) {
   if (VT != MVT::i32 && VT != MVT::i16 && VT != MVT::i8 && VT != MVT::i1)
     return 0;
   const TargetRegisterClass *RC = &Mips::GPR32RegClass;
   const ConstantInt *CI = cast<ConstantInt>(C);
   int64_t Imm;
-  if (CI->isNegative())
+  if ((VT != MVT::i1) && CI->isNegative())
     Imm = CI->getSExtValue();
   else
     Imm = CI->getZExtValue();
   return Materialize32BitInt(Imm, RC);
 }
 
 unsigned MipsFastISel::Materialize32BitInt(int64_t Imm,
                                            const TargetRegisterClass *RC) {

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

+; RUN: llc -march=mipsel -relocation-model=pic -O0 -mips-fast-isel -fast-isel-abort -mcpu=mips32r2 \
+; RUN:     < %s | FileCheck %s
+
+@.str = private unnamed_addr constant [6 x i8] c"hello\00", align 1
+@s = common global i8* null, align 4
+
+; Function Attrs: nounwind
+define void @foo() #0 {
+entry:
+  store i8* getelementptr inbounds ([6 x i8]* @.str, i32 0, i32 0), i8** @s, align 4
+  ret void
+; CHECK:        .ent    foo
+; CHECK:        lw      $[[REG1:[0-9]+]], %got($.str)(${{[0-9]+}})
+; CHECK:        addiu   ${{[0-9]+}}, $[[REG1]], %lo($.str)
+
+}
+
+attributes #0 = { nounwind "less-precise-fpmad"="false" "no-frame-pointer-elim"="false" "no-infs-fp-math"="false" "no-nans-fp-math"="false" "stack-protector-buffer-size"="8" "unsafe-fp-math"="false" "use-soft-float"="false" }
+