[PATCH] D14390: [mips] Expansion of LI.S and LI.D
Daniel Sanders via llvm-commits
llvm-commits at lists.llvm.org
Fri Dec 18 09:39:18 PST 2015
dsanders added a comment.
Please provide the whole context as per http://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface
In http://reviews.llvm.org/D14390#313777, @vkalintiris wrote:
> I believe that we can achieve the same thing without introducing new floating-point operands. We could add the `AsmToken::Real` case in the switch cases inside `parseImm()` & `parseOperand()`. This way the generic parser would parse the floats/doubles, saving the result in a int64_t type. After that it's just a matter of re-interpreting the bits to float or double depending on the instruction that we expand, and save the new bits in in64_t again. This would allow us to utilize the backtracking from the generic matcher in the future. Of course, we wouldn't be able to `print()` the operands with their types. However, we only care for their value, not their types, so there's no harm done.
I haven't fully read the patch yet but there's some high-level things to talk about.
There's a good reason to avoid adding the new MipsOperand kinds which is that it's important that the MipsOperand kinds do not overlap. We had some really big problems with this a year or two ago and I ended up rewriting a lot of the parsing and MipsOperand handling to resolve it. This patch currently re-introduces the root cause of those problems which I'm keen to avoid doing.
The problem with overlapping MipsOperand kinds is that there's only one chance to get the operand kind correct and there's no support for backtracking. In the original problem we had two 'add.d' instructions with one accepting FGR64 registers, and the other accepting AFGR64. These two register classes use the same set of names ($f0, $f1, ...). Our matcher table contained both possibilities with the FGR64 one appearing first. What happened was, the ParserMethod for FGR64 would be called and would create three MipsOperand of kind k_Register with, for example, registers D0, http://reviews.llvm.org/D1, and http://reviews.llvm.org/D2. It would then check the feature bits and reject this match because the feature bits said that the FPU was 32-bit (and therefore D0_64, D1_64, and D2_64 were needed instead). Then it would try the AFGR64 case but because there's no backtracking, we still had the same MipsOperand's. The PredicateMethod would always return false for these because D0/http://reviews.llvm.org/D1/http://reviews.llvm.org/D2 are not members of AFGR64 and we would therefore reject this match too. Having found no match, we would then reject the input and error out. Variants of this problem affected the majority of our instruction set in one way or another.
This problem almost manifests in this patch for inputs like:
li.s $2, 1
If it weren't for the ParserMethod's (which we ought to remove, we need to keep them to a minimum because they cause the above problems), the '1' would become a MipsOperand of kind k_Immediate which would never pass the PredicateMethod.
I'd approach this in a similar way to what Vasileios is describing but more towards the way k_RegisterIndex works. I'd have a single k_Immediate operand that separately holds integer, and APFloat values as well as a bitfield indicating which types are possible. I'd then have appropriate MipsOperand::Create*Imm functions that tell it which types are possible with '1' being valid for integer, float, and double, while '1.1' would only be valid for float and double. Finally, I'd have predicate methods that test the appropriate encoding (if it's valid) and a render method for each encoding to add the appropriately converted operand to the instruction.
This is a lot easier to explain with in person and with diagrams :-). Did that make sense?
================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:558
@@ -543,3 +557,3 @@
/// Potentially any (e.g. $1)
- RegKind_Numeric = RegKind_GPR | RegKind_FGR | RegKind_FCC | RegKind_MSA128 |
+ RegKind_Numeric = RegKind_GPR | RegKind_FCC | RegKind_MSA128 |
RegKind_MSACtrl | RegKind_COP2 | RegKind_ACC |
----------------
Why did you remove RegKind_FGR? Without it, you can't match things like:
mfc1 $2, $3
which is equivalent to:
mfc1 $2, $f3
================
Comment at: lib/Target/Mips/MCTargetDesc/MipsBaseInfo.h:128-216
@@ -124,2 +127,91 @@
+namespace llvm {
+
+namespace MIPS_FP {
+//===--------------------------------------------------------------------===//
+// Floating-point Immediates
+//
+static inline float getFPImmFloat(unsigned Imm) {
+ // We expect an 8-bit binary encoding of a floating-point number here.
+ union {
+ uint32_t I;
+ float F;
+ } FPUnion;
+
+ uint8_t Sign = (Imm >> 7) & 0x1;
+ uint8_t Exp = (Imm >> 4) & 0x7;
+ uint8_t Mantissa = Imm & 0xf;
+
+ // 8-bit FP iEEEE Float Encoding
+ // abcd efgh aBbbbbbc defgh000 00000000 00000000
+ //
+ // where B = NOT(b);
+
+ FPUnion.I = 0;
+ FPUnion.I |= Sign << 31;
+ FPUnion.I |= ((Exp & 0x4) != 0 ? 0 : 1) << 30;
+ FPUnion.I |= ((Exp & 0x4) != 0 ? 0x1f : 0) << 25;
+ FPUnion.I |= (Exp & 0x3) << 23;
+ FPUnion.I |= Mantissa << 19;
+ return FPUnion.F;
+}
+
+/// getFP32Imm - Return an 8-bit floating-point version of the 32-bit
+/// floating-point value. If the value cannot be represented as an 8-bit
+/// floating-point value, then return -1.
+static inline int getFP32Imm(const APInt &Imm) {
+ uint32_t Sign = Imm.lshr(31).getZExtValue() & 1;
+ int32_t Exp = (Imm.lshr(23).getSExtValue() & 0xff) - 127; // -126 to 127
+ int64_t Mantissa = Imm.getZExtValue() & 0x7fffff; // 23 bits
+
+ // We can handle 4 bits of mantissa.
+ // mantissa = (16+UInt(e:f:g:h))/16.
+ if (Mantissa & 0x7ffff)
+ return -1;
+ Mantissa >>= 19;
+ if ((Mantissa & 0xf) != Mantissa)
+ return -1;
+
+ // We can handle 3 bits of exponent: exp == UInt(NOT(b):c:d)-3
+ if (Exp < -3 || Exp > 4)
+ return -1;
+ Exp = ((Exp + 3) & 0x7) ^ 4;
+
+ return ((int)Sign << 7) | (Exp << 4) | Mantissa;
+}
+
+static inline int getFP32Imm(const APFloat &FPImm) {
+ return getFP32Imm(FPImm.bitcastToAPInt());
+}
+
+/// getFP64Imm - Return an 8-bit floating-point version of the 64-bit
+/// floating-point value. If the value cannot be represented as an 8-bit
+/// floating-point value, then return -1.
+static inline int getFP64Imm(const APInt &Imm) {
+ uint64_t Sign = Imm.lshr(63).getZExtValue() & 1;
+ int64_t Exp = (Imm.lshr(52).getSExtValue() & 0x7ff) - 1023; // -1022 to 1023
+ uint64_t Mantissa = Imm.getZExtValue() & 0xfffffffffffffULL;
+
+ // We can handle 4 bits of mantissa.
+ // mantissa = (16+UInt(e:f:g:h))/16.
+ if (Mantissa & 0xffffffffffffULL)
+ return -1;
+ Mantissa >>= 48;
+ if ((Mantissa & 0xf) != Mantissa)
+ return -1;
+
+ // We can handle 3 bits of exponent: exp == UInt(NOT(b):c:d)-3
+ if (Exp < -3 || Exp > 4)
+ return -1;
+ Exp = ((Exp + 3) & 0x7) ^ 4;
+
+ return ((int)Sign << 7) | (Exp << 4) | Mantissa;
+}
+
+static inline int getFP64Imm(const APFloat &FPImm) {
+ return getFP64Imm(FPImm.bitcastToAPInt());
+}
+} // end namespace MIPS_FP
+} // end namespace llvm
+
#endif
----------------
I don't understand this code. We don't have 8-bit floating point to my knowledge and li.s/li.d should be able to handle normal 32-bit and 64-bit floating point values respectively.
================
Comment at: lib/Target/Mips/MipsInstrFPU.td:560-579
@@ -535,1 +559,22 @@
+def LoadImmSingleGPR : MipsAsmPseudoInst<(outs GPR32Opnd
+ : $rd),
+ (ins fp_f32imm
+ : $fpimm),
+ "li.s\t$rd, $fpimm">;
+def LoadImmSingleFGR : MipsAsmPseudoInst<(outs FGR32Opnd
+ : $rd),
+ (ins fp_f32imm
+ : $fpimm),
+ "li.s\t$rd, $fpimm">;
+def LoadImmDoubleGPR : MipsAsmPseudoInst<(outs GPR32Opnd
+ : $rd),
+ (ins fp_f64imm
+ : $fpimm),
+ "li.d\t$rd, $fpimm">;
+def LoadImmDoubleFGR : MipsAsmPseudoInst<(outs FGR32Opnd
+ : $rd),
+ (ins fp_f64imm
+ : $fpimm),
+ "li.d\t$rd, $fpimm">;
+
----------------
Formatting. It looks like you may have used clang-format on a tablegen file which won't work correctly.
================
Comment at: test/MC/Mips/li.s.s:5-7
@@ +4,5 @@
+foo:
+li.s $4, 1.12345
+# CHECK: lui $4, 16271 # encoding: [0x8f,0x3f,0x04,0x3c]
+# CHECK: ori $4, $4, 52534 # encoding: [0x36,0xcd,0x84,0x34]
+
----------------
Hmm, it looks like we have some rounding here. I think it should be 0x3f8fcd35 but GAS emits the same output.
http://reviews.llvm.org/D14390
More information about the llvm-commits
mailing list