[PATCH] [mips] [IAS] Unify common functionality of LA and LI.
Toma Tabacu
toma.tabacu at imgtec.com
Wed May 6 10:04:55 PDT 2015
Replied to inline comments.
================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:1718-1720
@@ -1713,2 +1717,5 @@
-bool MipsAsmParser::expandLoadImm(MCInst &Inst, bool Is32BitImm, SMLoc IDLoc,
+bool MipsAsmParser::loadImmediate(const MCOperand *ImmOp,
+ const MCOperand *DstRegOp,
+ const MCOperand *SrcRegOp, bool Is32BitImm,
+ SMLoc IDLoc,
----------------
dsanders wrote:
> We don't really need the MCOperand's. We could have an int64_t for ImmOp and unsigned for the two register operands. This would also remove the need to allow nullptr in SrcRegOp since we could use Mips::NoRegister instead.
Done.
================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:1747
@@ -1738,1 +1746,3 @@
+ tmpInst.addOperand(MCOperand::CreateReg(DstReg));
+ tmpInst.addOperand(MCOperand::CreateReg(UseSrcReg ? SrcReg : Mips::ZERO));
tmpInst.addOperand(MCOperand::CreateImm(ImmValue));
----------------
dsanders wrote:
> Mips::ZERO should be Mips::ZERO64 on 64-bit targets.
>
> Likewise for the bitwise operations below but not for ADDiu.
>
> Also, I'd suggest having SrcReg default to Mips::ZERO or Mips::ZERO64 and renaming UseSrcReg to SrcRegIsNotZero.
Essentially done.
I didn't make SrcReg default to Mips::ZERO or Mips::ZERO64 because there are only 2 places where it could be used with those values and they are slightly different, as one of them should only get Mips::ZERO (the ADDiu).
This also means that there's no reason to rename UseSrcReg to SrcRegIsNotZero.
================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:1771-1778
@@ -1760,1 +1770,10 @@
+
+ if (UseSrcReg) {
+ tmpInst.clear();
+ tmpInst.setOpcode(Mips::ADDu);
+ tmpInst.addOperand(MCOperand::CreateReg(DstReg));
+ tmpInst.addOperand(MCOperand::CreateReg(DstReg));
+ tmpInst.addOperand(MCOperand::CreateReg(SrcReg));
+ Instructions.push_back(tmpInst);
+ }
} else if ((ImmValue & (0xffffLL << 48)) == 0) {
----------------
dsanders wrote:
> This could be done in a follow-up patch, but all of the cases below this one end with the same code to generate an ADDu. We could factor it out fairly easily.
Done.
================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:1869-1880
@@ -1831,1 +1868,14 @@
+bool MipsAsmParser::expandLoadImm(MCInst &Inst, bool Is32BitImm, SMLoc IDLoc,
+ SmallVectorImpl<MCInst> &Instructions) {
+ const MCOperand *ImmOp = &Inst.getOperand(1);
+ assert(ImmOp->isImm() && "expected immediate operand kind");
+ const MCOperand *DstRegOp = &Inst.getOperand(0);
+ assert(DstRegOp->isReg() && "expected register operand kind");
+
+ if (loadImmediate(ImmOp, DstRegOp, nullptr, Is32BitImm, IDLoc, Instructions))
+ return true;
+
+ return false;
+}
+
----------------
dsanders wrote:
> Similar to expandLoadAddressReg(), most of these pointers ought to be references and passed to loadImmediate() as, for example, &ImmOp or ImmOp->getImm()
Done.
================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:1883-1920
@@ -1832,89 +1882,40 @@
bool
-MipsAsmParser::expandLoadAddressReg(MCInst &Inst, SMLoc IDLoc,
+MipsAsmParser::expandLoadAddressReg(MCInst &Inst, bool Is32BitImm, SMLoc IDLoc,
SmallVectorImpl<MCInst> &Instructions) {
- MCInst tmpInst;
- const MCOperand &ImmOp = Inst.getOperand(2);
- assert((ImmOp.isImm() || ImmOp.isExpr()) &&
+ const MCOperand *ImmOp = &Inst.getOperand(2);
+ assert((ImmOp->isImm() || ImmOp->isExpr()) &&
"expected immediate operand kind");
- if (!ImmOp.isImm()) {
+ if (!ImmOp->isImm()) {
expandLoadAddressSym(Inst, IDLoc, Instructions);
return false;
}
- const MCOperand &SrcRegOp = Inst.getOperand(1);
- assert(SrcRegOp.isReg() && "expected register operand kind");
- const MCOperand &DstRegOp = Inst.getOperand(0);
- assert(DstRegOp.isReg() && "expected register operand kind");
- int ImmValue = ImmOp.getImm();
- if (-32768 <= ImmValue && ImmValue <= 65535) {
- // For -32768 <= j <= 65535.
- // la d,j(s) => addiu d,s,j
- tmpInst.setOpcode(Mips::ADDiu);
- tmpInst.addOperand(MCOperand::CreateReg(DstRegOp.getReg()));
- tmpInst.addOperand(MCOperand::CreateReg(SrcRegOp.getReg()));
- tmpInst.addOperand(MCOperand::CreateImm(ImmValue));
- Instructions.push_back(tmpInst);
- } else {
- // For any other value of j that is representable as a 32-bit integer.
- // la d,j(s) => lui d,hi16(j)
- // ori d,d,lo16(j)
- // addu d,d,s
- tmpInst.setOpcode(Mips::LUi);
- tmpInst.addOperand(MCOperand::CreateReg(DstRegOp.getReg()));
- tmpInst.addOperand(MCOperand::CreateImm((ImmValue & 0xffff0000) >> 16));
- Instructions.push_back(tmpInst);
- tmpInst.clear();
- tmpInst.setOpcode(Mips::ORi);
- tmpInst.addOperand(MCOperand::CreateReg(DstRegOp.getReg()));
- tmpInst.addOperand(MCOperand::CreateReg(DstRegOp.getReg()));
- tmpInst.addOperand(MCOperand::CreateImm(ImmValue & 0xffff));
- Instructions.push_back(tmpInst);
- tmpInst.clear();
- tmpInst.setOpcode(Mips::ADDu);
- tmpInst.addOperand(MCOperand::CreateReg(DstRegOp.getReg()));
- tmpInst.addOperand(MCOperand::CreateReg(DstRegOp.getReg()));
- tmpInst.addOperand(MCOperand::CreateReg(SrcRegOp.getReg()));
- Instructions.push_back(tmpInst);
- }
+ const MCOperand *SrcRegOp = &Inst.getOperand(1);
+ assert(SrcRegOp->isReg() && "expected register operand kind");
+ const MCOperand *DstRegOp = &Inst.getOperand(0);
+ assert(DstRegOp->isReg() && "expected register operand kind");
+
+ if (loadImmediate(ImmOp, DstRegOp, SrcRegOp, Is32BitImm, IDLoc, Instructions))
+ return true;
+
return false;
}
bool
-MipsAsmParser::expandLoadAddressImm(MCInst &Inst, SMLoc IDLoc,
+MipsAsmParser::expandLoadAddressImm(MCInst &Inst, bool Is32BitImm, SMLoc IDLoc,
SmallVectorImpl<MCInst> &Instructions) {
- MCInst tmpInst;
- const MCOperand &ImmOp = Inst.getOperand(1);
- assert((ImmOp.isImm() || ImmOp.isExpr()) &&
+ const MCOperand *ImmOp = &Inst.getOperand(1);
+ assert((ImmOp->isImm() || ImmOp->isExpr()) &&
"expected immediate operand kind");
- if (!ImmOp.isImm()) {
+ if (!ImmOp->isImm()) {
expandLoadAddressSym(Inst, IDLoc, Instructions);
return false;
}
- const MCOperand &RegOp = Inst.getOperand(0);
- assert(RegOp.isReg() && "expected register operand kind");
- int ImmValue = ImmOp.getImm();
- if (-32768 <= ImmValue && ImmValue <= 65535) {
- // For -32768 <= j <= 65535.
- // la d,j => addiu d,$zero,j
- tmpInst.setOpcode(Mips::ADDiu);
- tmpInst.addOperand(MCOperand::CreateReg(RegOp.getReg()));
- tmpInst.addOperand(MCOperand::CreateReg(Mips::ZERO));
- tmpInst.addOperand(MCOperand::CreateImm(ImmValue));
- Instructions.push_back(tmpInst);
- } else {
- // For any other value of j that is representable as a 32-bit integer.
- // la d,j => lui d,hi16(j)
- // ori d,d,lo16(j)
- tmpInst.setOpcode(Mips::LUi);
- tmpInst.addOperand(MCOperand::CreateReg(RegOp.getReg()));
- tmpInst.addOperand(MCOperand::CreateImm((ImmValue & 0xffff0000) >> 16));
- Instructions.push_back(tmpInst);
- tmpInst.clear();
- tmpInst.setOpcode(Mips::ORi);
- tmpInst.addOperand(MCOperand::CreateReg(RegOp.getReg()));
- tmpInst.addOperand(MCOperand::CreateReg(RegOp.getReg()));
- tmpInst.addOperand(MCOperand::CreateImm(ImmValue & 0xffff));
- Instructions.push_back(tmpInst);
- }
+ const MCOperand *DstRegOp = &Inst.getOperand(0);
+ assert(DstRegOp->isReg() && "expected register operand kind");
+
+ if (loadImmediate(ImmOp, DstRegOp, nullptr, Is32BitImm, IDLoc, Instructions))
+ return true;
+
return false;
}
----------------
dsanders wrote:
> Could you switch these back to references? You can always pass, for example, &ImmOp or ImmOp->getImm() into loadImmediate().
Done.
http://reviews.llvm.org/D9290
EMAIL PREFERENCES
http://reviews.llvm.org/settings/panel/emailpreferences/
More information about the llvm-commits
mailing list