Page MenuHomePhabricator

[mips] Expansion of SC[D] and LL[D] for R6
AbandonedPublic

Authored by atanasyan on Jan 28 2016, 1:17 AM.

Diff Detail

Event Timeline

obucina updated this revision to Diff 46243.Jan 28 2016, 1:17 AM
obucina retitled this revision from to [mips] Expansion of SC[D] and LL[D] for R6.
obucina updated this object.
obucina added reviewers: dsanders, zoran.jovanovic.
obucina added a subscriber: llvm-commits.
petarj edited edge metadata.Jan 28 2016, 6:52 AM
petarj added a subscriber: petarj.
sdardis requested changes to this revision.Jun 7 2016, 5:10 AM
sdardis edited edge metadata.

This patch needs to be rebased due to structural changes in the files it touches. emitRRRI and emitRRRX have to be moved to MCTargetDesc/MipsTargetStreamer.cpp, expandLlSc has to acquire and use MipsTargetStreamer for the various emit functions, along with passing the SubtargetInfo object around. The SmallVectorImp<MCInst> has been dropped.

Comments inlined. The major issue I see is that using loadImmediate to synthesise the address into a register causes the address to be truncated to 32bits due to the use of addu. This is OK for N32 and O32, but will cause failures on N64 if the address lies outside the high 32bits.

lib/Target/Mips/AsmParser/MipsAsmParser.cpp
227–229

The name IsMips64 is deceptive. I think this parameter should be renamed Is64bitAtomic as it controls whether the instruction being expanded is the 64bit variant.

1503–1521

This now goes in lib/Target/Mips/MCTargetDesc/MipsTargetStreamer.cpp

These should be members of MipsTargetStreamer. tmpInst in emitRRRX should be TmpInst for consistency with the rest of the file.

3213

The name IsMips64 is deceptive. It is perfectly valid to use ll+sc on mips64. I think this parameter should be renamed Is64bitAtomic.

3238

Daniel pointed this out to me, you can use isInt<9> rather than check the range manually.

3249

You can use isInt<16> rather than writing the range check by hand.

3259–3271

This is producing semi-correct code that only works for N32 on mips64r6.

With this patch, out of range ll/sc get expanded to:

24:   27a20108        addiu   v0,sp,264 <- wrong
28:   7c420037        lld     v0,0(v0)
2c:   64430001        daddiu  v1,v0,1
30:   27a10108        addiu   at,sp,264 <- wrong
34:   7c230027        scd     v1,0(at)

addiu cannot be used for 64 bit pointer manipulation as it's a 32 bit operation only. In contrast GAS generates:

24:   67a20108        daddiu  v0,sp,264
28:   7c420037        lld     v0,0(v0)
2c:   64430001        daddiu  v1,v0,1
30:   67a10108        daddiu  at,sp,264
34:   7c230027        scd     v1,0(at)

The fault lies with loadImmediate as it's treating the immediate as a 32bit value on a 64 bit machine.

lib/Target/Mips/Mips32r6InstrInfo.td
850

Align the assembly fragment to under the first '('.

853

Here too.

lib/Target/Mips/Mips64r6InstrInfo.td
222

Align the assembly fragment to under the first '('.

225

Here too.

test/MC/Mips/ll_r6.s
1

These test files should be called macro-<instruction>-r6.s

test/MC/Mips/lld_r6.s
1–53

All these addus are incorrect, they should be daddus.

test/MC/Mips/scd_r6.s
1–49

Same here, daddu instead of addu.

This revision now requires changes to proceed.Jun 7 2016, 5:10 AM
dsanders edited edge metadata.Jun 7 2016, 5:38 AM

One higher level comment I'd like to add to this is that I'm a somewhat reluctant to add support for the out-of-range offset macros that GAS supports. I currently think it has a very high cost in terms of implementation effort, testing effort, and maintenance for very little benefit. I'm not aware of any code that uses this feature intentionally and having IAS reject accidental usage of it can be beneficial in the case of ll/sc loops to avoid unnecessary instructions inside the loop.

dsanders resigned from this revision.Jul 12 2019, 4:15 PM
atanasyan commandeered this revision.Nov 27 2019, 2:37 AM
atanasyan abandoned this revision.
atanasyan added a reviewer: obucina.