This is an archive of the discontinued LLVM Phabricator instance.

[mips] Expansion of LI.S and LI.D
ClosedPublic

Authored by smaksimovic on Nov 5 2015, 1:02 PM.

Details

Summary

This patch introduces LI.S and LI.D pseudo instructions with floating point operands.

Diff Detail

Repository
rL LLVM

Event Timeline

obucina updated this revision to Diff 39399.Nov 5 2015, 1:02 PM
obucina retitled this revision from to [mips] Expansion of LI.S and LI.D.
obucina updated this object.
obucina added reviewers: dsanders, zoran.jovanovic.
obucina added a subscriber: llvm-commits.

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.

dsanders edited edge metadata.Dec 18 2015, 9:39 AM

Please provide the whole context as per http://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface

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, D1, and 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/D1/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?

lib/Target/Mips/AsmParser/MipsAsmParser.cpp
558 ↗(On Diff #39399)

Why did you remove RegKind_FGR? Without it, you can't match things like:

mfc1 $2, $3

which is equivalent to:

mfc1 $2, $f3
lib/Target/Mips/MCTargetDesc/MipsBaseInfo.h
128–216 ↗(On Diff #39399)

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.

lib/Target/Mips/MipsInstrFPU.td
560–579 ↗(On Diff #39399)

Formatting. It looks like you may have used clang-format on a tablegen file which won't work correctly.

test/MC/Mips/li.s.s
5–7 ↗(On Diff #39399)

Hmm, it looks like we have some rounding here. I think it should be 0x3f8fcd35 but GAS emits the same output.

obucina updated this revision to Diff 53556.Apr 13 2016, 7:50 AM
obucina edited edge metadata.

Expansion of LI.S and LI.D without introducing new floating-point operands.

obucina added inline comments.Apr 13 2016, 7:51 AM
lib/Target/Mips/AsmParser/MipsAsmParser.cpp
558 ↗(On Diff #53556)

RegKind_FGR is removed because we didn't find another way to distinguish RegKind_GPR and RegKind_FGR in
li.s $4 1.12345 and li.s $f4 1.12345, for example. Actually, for $4 in llvm is created RegKind_Numeric register, that is defined as RegKind_Numeric = RegKind_GPR | RegKind_FGR | RegKind_FCC ..., so condition isFGRAsmReg() in validateOperandClass() function (in MipsGenAsmMatcher.inc) is true for $4 and we get the expansion for RegKind_FGR instead of RegKind_GPR.

obucina updated this revision to Diff 54046.Apr 18 2016, 5:19 AM
obucina edited edge metadata.

Previous patch did not contain test cases. Added here.

dsanders requested changes to this revision.Apr 19 2016, 7:13 AM
dsanders edited edge metadata.
dsanders added inline comments.
lib/Target/Mips/AsmParser/MipsAsmParser.cpp
559 ↗(On Diff #54046)

RegKind_FGR is removed because we didn't find another way to distinguish RegKind_GPR and RegKind_FGR in li.s $4 1.12345 and li.s $f4 1.12345, for example.

This should work:

bool isStrictlyFGRAsmReg() { return isRegIdx() && RegIdx.Kind == RegKind_FGR && RegIdx.Index <= 31; }

This will only be true when an FGR is the only option (i.e. the source said '$f4'). You'll also need to subclass FGR32/FGR64/AFGR64 in tablegen, override the predicate method to isStrictlyFGRAsmReg, and use those new operands in the li.s/li.d pseudos.

Actually, for $4 in llvm is created RegKind_Numeric register, that is defined as RegKind_Numeric = RegKind_GPR | RegKind_FGR | RegKind_FCC ..., so condition isFGRAsmReg() in validateOperandClass() function (in MipsGenAsmMatcher.inc) is true for $4 and we get the expansion for RegKind_FGR instead of RegKind_GPR.

$4 is a RegKind_Numeric because $4 is ambiguous without additional context. It could be a A0, F4, D4, D4_64, FCC4, COP24 (register 4 in the COP2 set, we should probably add an underscore to the name), etc. depending on the mnemonic and which operand it appears in. The same is true of $f4 and RegKind_FGR to a lesser extent. It's still ambiguous which register '$f4' refers to and could be F4, D4, or D4_64 but it definitely isn't A0, COP24, etc.

The main thing here is that MipsOperand describes what the operand _might_ be rather than what it really is (we figure out what it _is_ at a later point in the assembler). With that in mind, MipsOperand::RegIdxOp::Kind is a bitfield representing all the possible interpretations of the operand by any instruction in our instruction set. There are some instructions where $4 is a floating point register, therefore RegKind_FGR must be part of RegKind_Numeric.

The ambiguity is resolved by the match table in the AsmMatcher. For each matchable, we call a particular predicate on each operand (specified in the tablegen definitions) and the first one to find that all the predicates are true is a match. When you have multiple matchables that can accept the same operands, the one that appears first in the table is chosen. I expect that LoadImmDoubleFGR appears first in your table so $4 is accepted there and LoadImmDoubleGPR never has the chance to match. We need to either make the two cases distinct (see isStrictlyFGRAsmReg() above) or control the sort order of the table.

lib/Target/Mips/MipsInstrFPU.td
574–588 ↗(On Diff #54046)

These should have the appropriate FGR_32, FGR_64, and HARDFLOAT adjectives.

586 ↗(On Diff #54046)

I don't think you mean FGR32Opnd here. You need FGR64Opnd for a 64-bit FPU and AFGR64Opnd for a 32-bit FPU.

This revision now requires changes to proceed.Apr 19 2016, 7:13 AM
mamidzic commandeered this revision.Aug 9 2016, 7:21 AM
mamidzic added a reviewer: obucina.
mamidzic edited edge metadata.Oct 5 2016, 2:24 AM
mamidzic updated this revision to Diff 73608.Oct 5 2016, 2:24 AM

Changes according to the review

Hi,

I'm unlikely to be able to look at this in the near future. I've added the new Mips code-owner (Simon) to the review.

sdardis requested changes to this revision.Oct 11 2016, 7:22 AM
sdardis edited edge metadata.

In several places there is the expression 'FirstReg + 1', This is unsafe as tablegen does not order the registers in the namespace as you would expect. Instead write a helper function to compute the next register.

Currently lib/Target/Mips/MipsGenRegisterInfo.inc in the build directory, the next register after a3 is ac0, not t0.

The second issue is that the mthc0 is only available for MIPS32r2 or later. For earlier revisions, the expansion is to use two mtcs, accessing the next numerical register and treating f0 as the "next register" in the f31 case.

lib/Target/Mips/AsmParser/MipsAsmParser.cpp
197–201 ↗(On Diff #73608)

There appears to be spurious white space on the line before this prototype, please remove it when you're committing.

921 ↗(On Diff #73608)

Here too.

956 ↗(On Diff #73608)

Whitespace here too.

2196 ↗(On Diff #73608)

Here as well.

2750–2756 ↗(On Diff #73608)

Please adjust the comment to say that this is a conversion of a double in an uint64_t to a float in a uint32_t, retaining the bit pattern of a float.

2805 ↗(On Diff #73608)

Indentation, this needs another space before the TOut.emitRRX(Mips::LWC1, ...

2830–2836 ↗(On Diff #73608)

Nm.

2874–2875 ↗(On Diff #73608)

Rather than using FirstReg + 1, instead write a helper function to compute the next register. It is unsafe to rely on tablegen ordering the registers in the expected manner.

Currently with FirstReg == Mips::A3, FirstReg + 1 is Mips::AC0, not Mips::T0.

lib/Target/Mips/MCTargetDesc/MipsMCCodeEmitter.cpp
714 ↗(On Diff #73608)

Spurious newline.

lib/Target/Mips/MipsInstrFPU.td
606–608 ↗(On Diff #73608)

This needs a HARDFLOAT predicate.

test/MC/Mips/macro-li.d.s
268 ↗(On Diff #73608)

Spurious whitespace at the end of this line.

test/MC/Mips/macro-li.s.s
32 ↗(On Diff #73608)

Spurious whitespace at the end of this line.

86 ↗(On Diff #73608)

Spurious whitespace at the end of this line.

This revision now requires changes to proceed.Oct 11 2016, 7:22 AM
sdardis added inline comments.Oct 11 2016, 7:24 AM
lib/Target/Mips/AsmParser/MipsAsmParser.cpp
197–201 ↗(On Diff #73608)

It's actually whitespace, not the line I'm concerned about.

921 ↗(On Diff #73608)

It's actually whitespace, not the line I'm concerned about.

mamidzic updated this revision to Diff 75861.Oct 26 2016, 5:51 AM
mamidzic edited edge metadata.

Addressed review comments.

sdardis added inline comments.Nov 3 2016, 4:25 AM
lib/Target/Mips/AsmParser/MipsAsmParser.cpp
2769 ↗(On Diff #75861)

Two things:

This function also needs to handle floating point registers.

Rather than returning 0, instead call llvm_unreachable("Unknown register in assembly macro expansion!");

2929 ↗(On Diff #75861)

This block has the wrong condition. This should check that the lower 32 bits are zero and the high part can be loaded with a single instruction.

2931–2936 ↗(On Diff #75861)

Add a FIXME comment here noting that in the case where the constant is zero, we can load the register directly from the zero register.

2933 ↗(On Diff #75861)

This condition is not required.

2937 ↗(On Diff #75861)

This also needs a check for hasMips64() before checking hasMips32r2(). In the mips64 case, we use dmtc1.

2938–2939 ↗(On Diff #75861)

If you've reverse engineered this from the output of gas, you've hit upon a latent gas bug.

On a MIPS32 system (or a MIPS64 system executing MIPS32 code) , writing to a 64 bit FPU register must be done in a specific order; use mtc1 to write the lower 32bits, then use mthc1 to write the upper 32 bits. mtcX instructions leave the upper 32bits *UNPREDICTABLE*.

Reverse the order of these two lines.

2941–2942 ↗(On Diff #75861)

See my comment about nextReg.

Also, indentation is incorrect.

2979–2980 ↗(On Diff #75861)

Rather than Mips::LDC1, this should be (IsFPU64 ? Mips::LDC164 : Mips::LDC1) to get the correct instruction.

Add a FIXME comment here noting that this expansion is incorrect for mips1, it should expand to two word loads.

test/MC/Mips/macro-li.d.s
1 ↗(On Diff #75861)

Can you also a RUN line for mips32 and add appropriate tests?

sdardis requested changes to this revision.Nov 3 2016, 4:26 AM
sdardis edited edge metadata.
This revision now requires changes to proceed.Nov 3 2016, 4:26 AM
mamidzic updated this revision to Diff 80237.Dec 5 2016, 1:31 AM
mamidzic edited edge metadata.

Addressed the review comments.

sdardis requested changes to this revision.Dec 12 2016, 6:50 AM
sdardis edited edge metadata.

I missed something the first time around reviewing this. The usage of the .lit4 & .lit8 is conditional on the usage of the sdata section. For now, you can removed the .litX handling and just use .rodata in all cases. There's a longer explanation inline.

Thanks,
Simon

lib/Target/Mips/AsmParser/MipsAsmParser.cpp
2736–2737 ↗(On Diff #80237)

This is almost correct. GAS will assemble "li.d $f31, 2.5" into "lui $at, 0x4004; mtc $at, $f0; mtc $zero, $f31".

You'll need to expand out the check or check for that specific case.

2771 ↗(On Diff #80237)

Stray space the end of the line.

2843–2849 ↗(On Diff #80237)

I missed this the first time around. The usage of .lit4 & .lit8 is permitted when: a) the small data section is in use, and b) the size of the constant is within the size threshold for the small data section.

If the small data section cannot be used, the constant is located within the .rodata section.

For the moment, just change this to always use the .rodata section, and add "FIXME: Enhance this expansion to use the .lit4 & .lit8 sections where appropriate."

This avoids having to modify MipsTargetObjectFile.cpp with arguably unrelated changes.

2968–2976 ↗(On Diff #80237)

See my comment about the .lit4 section.

test/MC/Mips/macro-li.d.s
284 ↗(On Diff #80237)

Please put a newline at the end of the file.

This revision now requires changes to proceed.Dec 12 2016, 6:50 AM
smaksimovic commandeered this revision.Feb 9 2017, 5:25 AM
smaksimovic added a reviewer: mamidzic.
smaksimovic edited edge metadata.

Addressed review comments.

sdardis requested changes to this revision.Feb 15 2017, 9:13 AM

A few small things. Mostly the changes necessary are along the lines of differentiating between PIC and non-PIC. I've written them inline. The other minor change is that you should remove the stub implementation of the literal section and submit that afterwards. It will be easier to implement/review then.

lib/Target/Mips/AsmParser/MipsAsmParser.cpp
2980–2986 ↗(On Diff #88364)

Rather than going through void pointers, use BitsToDouble & co. from llvm/Support/MathExtras.h.

3021–3038 ↗(On Diff #88364)

This section isn't quite right.

In the PIC case, we have to load the address of the constant via the GOT, with a R_MIPS_GOT16 relocation attached to an lw. Then load the constant with ldc with a relocation type R_MIPS_LO16 attached to it.

In the non-pic case, it's lui with a R_MIPS_HI16 relocation, then a ldc1 with a R_MIPS_LO16.

3052–3057 ↗(On Diff #88364)

This is the expected behaviour for loading a 64bit value in 32bit GPRs. For 64 bit GPRs we have to load it into a single register. Test the ABI to determine if we should load into one or two registers.

3081–3107 ↗(On Diff #88364)

This hunk needs to be re-arranged slightly. The first step is to generate an emit the upper portion of the constant's address for non-PIC or load the address of the the constant from the GOT. Both cases set up $at.

The second step is to build an expression that refers to the base address of the constant.

Then finally, if the ABI is N32 or N64 perform emit ld, otherwise since it's O32, emit two loads.

3121 ↗(On Diff #88364)

This should be

if (isABI_N32() || isABI_N64())

as the instruction expansion is dependant on the ABI.

3151–3167 ↗(On Diff #88364)

This section isn't quite right.

In the PIC case, we have to load the address of the constant via the GOT, with a R_MIPS_GOT16 relocation attached to an ld. Then load the constant with ldc with a relocation type R_MIPS_LO16 attached to it.

In the non-pic case, it's lui with a R_MIPS_HI16 relocation, then a ldc1 with a R_MIPS_LO16.

test/MC/Mips/macro-li.s
1 ↗(On Diff #88364)

This file can be kept, it's testing a different macro.

This revision now requires changes to proceed.Feb 15 2017, 9:13 AM
smaksimovic edited edge metadata.

Made additional ABI checks when loading floats to FPRs, doubles to GPRs and doubles to FPRs.
We've mostly compared it to GCC, the caveat being that GCC generates floats using lui/ori in GPRs and then moves them to FPRs with mtc1 if necessary,
whereas we always place constants into .rodata, and have relocations accordingly.

Another difference is that despite invoking GCC without -fpic flag for n64, it generates code that has R_MIPS_GOT16 relocations.
Implemented that part to use %higher, %highest relocations to fetch the address.

sdardis requested changes to this revision.May 4 2017, 9:13 AM

This needs a bit more work. The problem is that the address for the constant loads is the address of the .rodata section, not the address of the constant in the .rodata section. I've spelled out the changes required inline.

lib/Target/Mips/AsmParser/MipsAsmParser.cpp
3061 ↗(On Diff #97068)

Can you make this a private class member of MipsAsmParser instead?

It avoids having to explicitly pass STI, ATReg, IsPicEnabled.

3162–3176 ↗(On Diff #97068)

See my comment about generating the addresses correctly.

3210–3222 ↗(On Diff #97068)

See my comment about generating addresses correctly.

3262–3274 ↗(On Diff #97068)

This two hunks need to be reversed and modified slightly.

The problem is that you're generating the symbol with the name of the ReadOnlySection. The linker will resolve this as to the start of the ReadOnlySection and rewrite the relocs to that value.

Instead, you need to create a symbol visible to this object, switch to the ReadOnlySection, then emit the symbol and data.

test/MC/Mips/macro-li.d.s
1 ↗(On Diff #97068)

Small change here for both files. Can you use the label O32-N32-(NO-)PIC as required? It makes to more obvious that the check lines are for O32 and N32.

This revision now requires changes to proceed.May 4 2017, 9:13 AM
smaksimovic edited edge metadata.

Addressed review comments.

sdardis accepted this revision.May 10 2017, 6:41 AM

LGTM with the fixmes added and the whitespace nit addressed.

lib/Target/Mips/AsmParser/MipsAsmParser.cpp
3088 ↗(On Diff #98253)

Sorry, I missed something here. For O32 and N32 all addresses are 32 bits. For N64 addresses are 64 bits unless -msym32 is used--in which case symbols are 32 bits.

Unfortunately, I've found something questionable in the GNU assembler which is that li.d always assumes that the address of the temporary symbol used to load the constant is always 32 bits in the non-pic case.

Can you attach two fixmes here? The first is that our assembler is technically correct but gives a different result to gas but gas is incomplete there (it has a fixme noting it doesn't work with 64-bit addresses). The second is that with -msym32 the address expansion for N64 should probably use the O32 / N32 case. It's safe to use the 64 address expansion as the symbol's value is considered sign extended.

3225–3226 ↗(On Diff #98253)

FIXME: This method is too general. In principal we should compute the number of instructions required to synthesize the immediate inline compared to synthesising the address inline and relying on non .text sections. For static O32 and N32 this may yield a small benefit, for static N64 this is likely to yield a much larger benefit as we have to synthesize a 64bit address to load a 64 bit value.

test/MC/Mips/macro-li.d.s
356 ↗(On Diff #98253)

Spurious whitespace at the end of this line.

We'll need @dsanders 's opinion too.

dsanders accepted this revision.May 10 2017, 7:06 AM

I haven't looked too closely at the output of the macro since I don't know what it should be but it looks like the issues I raised earlier in the review are no longer there. I just have one nit about nextReg().

lib/Target/Mips/AsmParser/MipsAsmParser.cpp
3007 ↗(On Diff #98253)

I'd recommend a comment mentioning the D0 + 1 == F1 and F1 + 1 == D1 quirks. It makes sense when you're thinking of it in the context of where it's being used but F1 + 1 == D1 in particular isn't very obvious without that context. It would be reasonable for a reader to expect F1 + 1 == F2.

This revision is now accepted and ready to land.May 10 2017, 7:06 AM

Removed whitespace, added fixmes and a comment.

smaksimovic edited the summary of this revision. (Show Details)May 12 2017, 3:00 AM
This revision was automatically updated to reflect the committed changes.