This is an archive of the discontinued LLVM Phabricator instance.

[mips] [IAS] Add partial support for the ULHU pseudo-instruction.
ClosedPublic

Authored by tomatabacu on May 11 2015, 10:36 AM.

Details

Summary

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.

Diff Detail

Event Timeline

tomatabacu retitled this revision from to [mips] [IAS] Add partial support for the ULHU pseudo-instruction..
tomatabacu updated this object.
tomatabacu edited the test plan for this revision. (Show Details)
tomatabacu added a reviewer: dsanders.
tomatabacu added a subscriber: Unknown Object (MLST).
dsanders added inline comments.May 12 2015, 8:18 AM
lib/Target/Mips/AsmParser/MipsAsmParser.cpp
2164–2165

Nit: We're going to end up writing this a lot. It would be good to factor it out.

2181–2182

For '.set noat' mode we currently give up here before discovering whether we need AT or not.

2188

What happens if OffsetValue is -32769? OffsetValue+1 will fit but we still need AT for OffsetValue

2203–2215

Shouldn't endianness have some effect on the expansion?

2204–2220

Nit: There's some repetition in the ternary operators. Could you factor out the repeated ones into variables?

2254

I'm not sure why this is needed.

dsanders requested changes to this revision.May 12 2015, 9:32 AM
dsanders edited edge metadata.
This revision now requires changes to proceed.May 12 2015, 9:32 AM
tomatabacu updated this revision to Diff 26226.May 21 2015, 5:56 AM
tomatabacu edited edge metadata.

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.

tomatabacu added inline comments.May 21 2015, 5:58 AM
lib/Target/Mips/AsmParser/MipsAsmParser.cpp
2164–2165

Done in rL237780.

2181–2182

We always need AT for ULHU, as it is always used as the source register for one of the LBu's.
I added a code comment with this explanation.

2188

Fixed and added as test case in mips-expansions.s.
Also added a test case for the inverse case, when OffsetValue fits but OffsetValue+1 doesn't (OffsetValue is 32767).

2203–2215

Yes, it should. Fixed and added checks for both endian in mips-expansions.s.

2204–2220

Done.

2254

Because otherwise we would always generate ADDu, and GAS generates a DADDu for MIPS64.
The reason why it's in this patch is because this is where we have a concrete test case for it.

tomatabacu edited edge metadata.

Rebased.

dsanders requested changes to this revision.Jun 12 2015, 6:21 AM
dsanders edited edge metadata.
dsanders added inline comments.
lib/Target/Mips/AsmParser/MipsAsmParser.cpp
2181–2182

Ah yes, it looked like one path didn't use it but that's not the case.

2204

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.

2254

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

This revision now requires changes to proceed.Jun 12 2015, 6:21 AM
tomatabacu updated this revision to Diff 28028.Jun 19 2015, 7:45 AM
tomatabacu edited edge metadata.

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.

dsanders accepted this revision.Jun 23 2015, 6:23 AM
dsanders edited edge metadata.

I agree we've covered all the comments. LGTM

lib/Target/Mips/AsmParser/MipsAsmParser.cpp
2197–2199

I found this clarification a bit confusing at first. It might be better to write out the instructions.

This revision is now accepted and ready to land.Jun 23 2015, 6:23 AM
tomatabacu closed this revision.Jun 23 2015, 7:44 AM