This patch adds the seq macro.
This partially resolves PR/30381.
Thanks to Sean Bruno for reporting the issue!
Paths
| Differential D24607
[mips] seq macro support
ClosedPublic Authored by sdardis on Sep 15 2016, 5:20 AM.
Details Summary This patch adds the seq macro. This partially resolves PR/30381. Thanks to Sean Bruno for reporting the issue!
Diff Detail Event Timelinesdardis updated this object. Comment Actions Looks right to me. I think you can split the seq and not changes into two separate commits? Comment Actions This seems to not cleanly apply and has a couple of rejects: sbruno@tasty.ysv:~/clang/llvm % find . -name '*.rej' ./test/CodeGen/Mips/fcopysign-f32-f64.ll.rej ./lib/Target/Mips/AsmParser/MipsAsmParser.cpp.rej ./lib/Target/Mips/MipsInstrInfo.td.rej sbruno@tasty.ysv:~/clang/llvm % cat ./lib/Target/Mips/AsmParser/MipsAsmParser.cpp.rej @@ -2213,6 +2219,10 @@ return expandDRotationImm(Inst, IDLoc, Out, STI) ? MER_Fail : MER_Success; case Mips::ABSMacro: return expandAbs(Inst, IDLoc, Out, STI) ? MER_Fail : MER_Success; + case Mips::SEQMacro: + return expandSeq(Inst, IDLoc, Out, STI) ? MER_Fail : MER_Success; + case Mips::SEQIMacro: + return expandSeqI(Inst, IDLoc, Out, STI) ? MER_Fail : MER_Success; } } sbruno@tasty.ysv:~/clang/llvm % cat ./lib/Target/Mips/MipsInstrInfo.td.rej @@ -201,6 +201,8 @@ AssemblerPredicate<"FeatureMips16">; def HasCnMips : Predicate<"Subtarget->hasCnMips()">, AssemblerPredicate<"FeatureCnMips">; +def NotCnMips : Predicate<"!Subtarget->hasCnMips()">, + AssemblerPredicate<"!FeatureCnMips">; def RelocNotPIC : Predicate<"!TM.isPositionIndependent()">; def RelocPIC : Predicate<"TM.isPositionIndependent()">; def NoNaNsFPMath : Predicate<"TM.Options.NoNaNsFPMath">; This revision now requires changes to proceed.Sep 15 2016, 7:59 AM Comment Actions What revision are you trying it against? It's applying mostly clean for me against rL281613. Only the changes to MipsAsmParser.cpp had a offset change (-2). Comment Actions ~/clang/llvm % svn info . Path: . Working Copy Root Path: /home/sbruno/clang/llvm URL: http://llvm.org/svn/llvm-project/llvm/trunk Relative URL: ^/llvm/trunk Repository Root: http://llvm.org/svn/llvm-project Repository UUID: 91177308-0d34-0410-b5e6-96231b3b80d8 Revision: 281608 Node Kind: directory Schedule: normal Last Changed Author: sdardis Last Changed Rev: 281607 Last Changed Date: 2016-09-15 13:13:01 +0000 (Thu, 15 Sep 2016) sdardis updated this object. sdardis edited edge metadata. Comment ActionsRemoved 'not' alias, will add that as a separate patch. sdardis retitled this revision from [mips] seq macro support and not alias
to [mips] seq macro support
.Sep 19 2016, 1:56 AM sdardis edited edge metadata. seanbruno edited edge metadata. Comment ActionsI'm unsure if this is a patch/phabricator messing up or if this patch needs to be regenerated. ~/clang/llvm % patch -p0 < ../D24607.diff Hmm... Looks like a unified diff to me... The text leading up to this was: -------------------------- |Index: lib/Target/Mips/AsmParser/MipsAsmParser.cpp |=================================================================== |--- lib/Target/Mips/AsmParser/MipsAsmParser.cpp |+++ lib/Target/Mips/AsmParser/MipsAsmParser.cpp -------------------------- Patching file lib/Target/Mips/AsmParser/MipsAsmParser.cpp using Plan A... Hunk #1 succeeded at 252 with fuzz 2 (offset 3 lines). Hunk #2 failed at 2217. Hunk #3 succeeded at 3909 (offset 83 lines). 1 out of 3 hunks failed--saving rejects to lib/Target/Mips/AsmParser/MipsAsmParser.cpp.rej Hmm... The next patch looks like a unified diff to me... The text leading up to this was: -------------------------- |Index: lib/Target/Mips/MipsInstrInfo.td |=================================================================== |--- lib/Target/Mips/MipsInstrInfo.td |+++ lib/Target/Mips/MipsInstrInfo.td -------------------------- Patching file lib/Target/Mips/MipsInstrInfo.td using Plan A... Hunk #1 failed at 201. Hunk #2 succeeded at 351 (offset 27 lines). Hunk #3 succeeded at 2251 (offset 27 lines). 1 out of 3 hunks failed--saving rejects to lib/Target/Mips/MipsInstrInfo.td.rej I can hand edit to get this patch in, but thought it was worth noting. This revision now requires changes to proceed.Sep 19 2016, 8:48 AM Comment Actions This patch needs to be regenerated against the top of tree at the moment. I've had to manually modify the code for the following files recently to test this: ./lib/Target/Mips/AsmParser/MipsAsmParser.cpp.rej Comment Actions That's odd. OSX's patch (2.5.8) reports that hunks #2, #3 apply with offsets 8 and 29 lines respectively. For MipsAsmPArser, #2 and #3 apply with 5 and 3 respectively. This is against rL283312. Is your version of patch sufficiently different? Comment Actions Possibly. I'm not going to stress out about it and you're probably going to be committing this soonish? vkalintiris edited edge metadata. Comment ActionsWe generate different code from the GNU assembler which in some cases is wrong, see inline comment.
sdardis edited edge metadata. sdardis marked an inline comment as done. Comment ActionsAddress review comments. vkalintiris edited edge metadata. Comment ActionsI didn't consider all the cases but it seems that the macro expansion is probably fine in terms of correctness. However, the expansion is different from that of GAS' and this will generate different object files unnecessarily. As far as I can tell, GAS handles these macro in 3 different ways depending on the value of the immediate:
This revision now requires changes to proceed.Oct 25 2016, 7:49 AM seanbruno edited edge metadata. Comment ActionsCurrently required to build the FreeBSD MALTA mips kernels. Thanks for working on this! This revision is now accepted and ready to land.Nov 21 2016, 11:28 AM Closed by commit rL287573: [mips] seq macro support (authored by sdardis). · Explain WhyNov 21 2016, 12:40 PM This revision was automatically updated to reflect the committed changes.
Revision Contents
Diff 71494 lib/Target/Mips/AsmParser/MipsAsmParser.cpp
lib/Target/Mips/MipsInstrInfo.td
test/MC/Mips/macro-seq.s
test/MC/Mips/mips1/valid.s
test/MC/Mips/mips2/valid.s
test/MC/Mips/mips3/valid.s
test/MC/Mips/mips32/valid.s
test/MC/Mips/mips32r2/valid.s
test/MC/Mips/mips32r3/valid.s
test/MC/Mips/mips32r5/valid.s
test/MC/Mips/mips32r6/valid.s
test/MC/Mips/mips4/valid.s
test/MC/Mips/mips5/valid.s
test/MC/Mips/mips64/valid.s
test/MC/Mips/mips64r2/valid.s
test/MC/Mips/mips64r3/valid.s
test/MC/Mips/mips64r5/valid.s
test/MC/Mips/mips64r6/valid.s
|
The constants manipulation here looks problematic, eg:
With gas:
With llvm-mc:
Similarly for:
gas:
llvm-mc:
Also, on 64-bit we should generate a daddiu instead of an addiu according to the gas testsuite in binutils.