This only adds support for ULHU of an immediate address with/without a source register.
It does not include support for ULHU of the address of a symbol.
Details
Diff Detail
Event Timeline
lib/Target/Mips/AsmParser/MipsAsmParser.cpp | ||
---|---|---|
2191–2192 | Nit: We're going to end up writing this a lot. It would be good to factor it out. | |
2208–2209 | For '.set noat' mode we currently give up here before discovering whether we need AT or not. | |
2215 | What happens if OffsetValue is -32769? OffsetValue+1 will fit but we still need AT for OffsetValue | |
2230–2242 | Shouldn't endianness have some effect on the expansion? | |
2231–2247 | Nit: There's some repetition in the ternary operators. Could you factor out the repeated ones into variables? | |
2297 | I'm not sure why this is needed. |
Addressed review comments.
Moved generation of (D)ADDu locally in order to closer match GAS' output.
Added .set nomacro warning and test.
Reformatted mips-expansions.s.
Removed some generation checks to fix mips-expansions-bad.s issues caused by rebase.
Rebased.
lib/Target/Mips/AsmParser/MipsAsmParser.cpp | ||
---|---|---|
2191–2192 | Done in rL237780. | |
2208–2209 | We always need AT for ULHU, as it is always used as the source register for one of the LBu's. | |
2215 | Fixed and added as test case in mips-expansions.s. | |
2230–2242 | Yes, it should. Fixed and added checks for both endian in mips-expansions.s. | |
2231–2247 | Done. | |
2297 | Because otherwise we would always generate ADDu, and GAS generates a DADDu for MIPS64. |
lib/Target/Mips/AsmParser/MipsAsmParser.cpp | ||
---|---|---|
2208–2209 | Ah yes, it looked like one path didn't use it but that's not the case. | |
2231 | Nit: I believe Fst should be First and Snd should be Second. Likewise below FWIW, I'd usually spell them 'TmpReg1' and 'TmpReg2'. There's often little description you can give to scratch registers. | |
2297 | Hmm.. This patch uses loadImmediate() is to load an offset into a register. In this context, we don't want to test for 64-bit GPR's but rather for 64-bit pointers (see MipsABIInfo::GetPtrAddiuOp()). However, loadImmediate() is also used for the 'li' instruction where we do want the test to be for 64-bit GPR's. You'll need to separate the two contexts somehow. I think line 2197 ought to be: if (loadImmediate(OffsetValue, ATReg, SrcReg, ABI.ArePtrs64bit(), IDLoc, Instructions)) and the Is32bitImm argument to loadImmediate() needs to determine whether you get ADDu or DADDu |
Addressed review comments (one of them resulted in D10568).
Added a code comment with an example of why we add the SrcReg locally.
Fixed mistake in test/MC/Mips/set-nomacro.s caused by a previous rebase.
I agree we've covered all the comments. LGTM
lib/Target/Mips/AsmParser/MipsAsmParser.cpp | ||
---|---|---|
2224–2226 | I found this clarification a bit confusing at first. It might be better to write out the instructions. |
Nit: We're going to end up writing this a lot. It would be good to factor it out.