Page MenuHomePhabricator

[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

Repository
rL LLVM

Event Timeline

sdardis updated this revision to Diff 71494.Sep 15 2016, 5:20 AM
sdardis retitled this revision from to [mips] seq macro support and not alias .
sdardis updated this object.
sdardis set the repository for this revision to rL LLVM.
sdardis added subscribers: llvm-commits, seanbruno.
emaste added a subscriber: emaste.Sep 15 2016, 7:02 AM

Looks right to me. I think you can split the seq and not changes into two separate commits?

seanbruno requested changes to this revision.Sep 15 2016, 7:59 AM
seanbruno added a reviewer: seanbruno.

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

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).

~/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 revision to Diff 71780.Sep 19 2016, 1:56 AM
sdardis updated this object.
sdardis edited edge metadata.
sdardis removed rL LLVM as the repository for this revision.

Removed '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 requested changes to this revision.Sep 19 2016, 8:48 AM
seanbruno edited edge metadata.

I'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

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
./lib/Target/Mips/MipsInstrInfo.td.rej

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?

Possibly. I'm not going to stress out about it and you're probably going to be committing this soonish?

Yes, @vkalintiris should be reviewing this soon hopefully.

vkalintiris requested changes to this revision.Oct 19 2016, 6:13 AM
vkalintiris edited edge metadata.

We generate different code from the GNU assembler which in some cases is wrong, see inline comment.

lib/Target/Mips/AsmParser/MipsAsmParser.cpp
3862–3878 ↗(On Diff #71780)

The constants manipulation here looks problematic, eg:

seq     $4, $5, -66666

With gas:

lui     at,0xfffe
ori     at,at,0xfb96
xor     a0,a1,at
sltiu   a0,a0,1

With llvm-mc:

addiu   $4, $5, 1130
sltiu   $4, $4, 1

Similarly for:

seq     $4, $5, -2147483648

gas:

lui     at,0x8000
xor     a0,a1,at 
sltiu   a0,a0,1

llvm-mc:

addiu   $4, $5, 0
sltiu   $4, $4, 1

Also, on 64-bit we should generate a daddiu instead of an addiu according to the gas testsuite in binutils.

sdardis marked an inline comment as done.Oct 19 2016, 7:01 AM
sdardis added inline comments.
lib/Target/Mips/AsmParser/MipsAsmParser.cpp
3862–3878 ↗(On Diff #71780)

Right. It appears that seqi reads immediates like li does. I have a working copy with this addressed and I'll add your examples to the test.

sdardis updated this revision to Diff 75158.Oct 19 2016, 8:40 AM
sdardis edited edge metadata.
sdardis marked an inline comment as done.

Address review comments.

vkalintiris requested changes to this revision.Oct 25 2016, 7:49 AM
vkalintiris edited edge metadata.

I 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:

  1. imm == 0 -> generate just an sltiu,
  2. op[1] == 0 -> generate warning and set op[0] = 0
    • imm in [0, 0x10000) -> xori
    • imm in (-0x8000, 0) -> imm = -imm AND {,d}addiu
    • for every other imm -> loadImmediate && xor.
    • finally emit sltiu.
This revision now requires changes to proceed.Oct 25 2016, 7:49 AM
sdardis updated this revision to Diff 76421.Oct 31 2016, 9:09 AM
sdardis edited edge metadata.

Addressed review comments.

vkalintiris accepted this revision.Nov 2 2016, 10:59 AM
vkalintiris edited edge metadata.

LGTM but there's a wrong constant that you should change before committing this.

lib/Target/Mips/AsmParser/MipsAsmParser.cpp
3977 ↗(On Diff #76421)

We want the -0x8000 constant literal here.

4316–4320 ↗(On Diff #76421)

Can you fix the identation here?

seanbruno accepted this revision.Nov 21 2016, 11:28 AM
seanbruno edited edge metadata.

Currently 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
This revision was automatically updated to reflect the committed changes.