Details
Diff Detail
Event Timeline
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. | |
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.
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.