This is an archive of the discontinued LLVM Phabricator instance.

Sparc: Implement i64 load/store support for 32-bit sparc.
ClosedPublic

Authored by jyknight on Mar 30 2015, 3:31 PM.

Details

Summary

The LDD/STD instructions can load/store a 64bit quantity from/to
memory to/from a consecutive even/odd pair of (32-bit) registers. They
are part of SparcV8, and also present in SparcV9. (Although deprecated
there, as you can store 64bits in one register).

As recommended on llvmdev in the thread "How to enable use of 64bit
load/store for 32bit architecture" from Apr 2015, I've modeled the
64-bit load/store operations as working on a v2i32 type, rather than
making i64 a legal type, but with few legal operations. The latter
does not (currently) work, as there is much code in llvm which assumes
that if i64 is legal, operations like "add" will actually work on it.

The same assumption does not hold for v2i32 -- for vector types, it is
workable to support only load/store, and expand everything else.

This patch:

  • Adds a new register class, IntPair, for even/odd pairs of registers.
  • Modifies the list of reserved registers, the stack spilling code, and register copying code to support the IntPair register class.
  • Adds support in AsmParser. (note that in asm text, you write the name of the first register of the pair only. So the parser has to morph the single register into the equivalent paired register).
  • Adds the new instructions themselves (LDD/STD/LDDA/STDA).
  • Hooks up the instructions and registers as a vector type v2i32. Adds custom legalizer to transform i64 load/stores into v2i32 load/stores and bitcasts, so that the new instructions can actually be generated, and marks all operations other than load/store on v2i32 as needing to be expanded.
  • Copies the unfortunate SelectInlineAsm hack from ARMISelDAGToDAG. This hack undoes the transformation of i64 operands into two arbitrarily-allocated separate i32 registers in SelectionDAGBuilder. and instead passes them in a single IntPair. (Arbitrarily allocated registers are not useful, asm code expects to be receiving a pair, which can be passed to ldd/std.)

Also adds a bunch of test cases covering all the bugs I've added along
the way.

Diff Detail

Repository
rL LLVM

Event Timeline

jyknight updated this revision to Diff 22912.Mar 30 2015, 3:31 PM
jyknight retitled this revision from to Sparc: Implement LDD/STD double-word load/store instructions..
jyknight updated this object.
jyknight edited the test plan for this revision. (Show Details)
jyknight set the repository for this revision to rL LLVM.
jyknight added a subscriber: Unknown Object (MLST).
jyknight updated this revision to Diff 23438.Apr 8 2015, 1:13 PM

Implemented support for int64 stores/loads using the new instructions.

jyknight updated this object.Apr 8 2015, 1:14 PM
jyknight updated this revision to Diff 23963.Apr 17 2015, 1:33 PM
jyknight updated this object.

Adds more test cases.
Fixes inline asm register constraint register class selection.
Fixes remapRegsForLeafProc.

jyknight updated this revision to Diff 25709.May 13 2015, 10:27 AM

Discovered and fixed a bunch of bugs and added a bunch more test cases.

jyknight retitled this revision from Sparc: Implement LDD/STD double-word load/store instructions. to Sparc: Implement i64 load/store support for 32-bit sparc. .May 13 2015, 10:29 AM
jyknight updated this object.
jyknight edited edge metadata.
t.p.northover added inline comments.
lib/Target/Sparc/SparcFrameLowering.cpp
232–235 ↗(On Diff #25709)

Don't you need to mark the pair unused as well? I wouldn't swear to this, but a quick look suggests that marking I0_I1 unused would implicitly mark I0 and I1 unused, but not the converse.

245 ↗(On Diff #25709)

Is this delta correct? I'd have expected "reg - SP::I0_I1 + SP::O0_O1" unless I'm misunderstanding the logic.

lib/Target/Sparc/SparcISelLowering.cpp
2641–2642 ↗(On Diff #25709)

I don't understand this: it's the only place a 128-bit type is mentioned at all in the patch. Why does the default action change?

3313 ↗(On Diff #25709)

Commented code.

Thanks a bunch for your notes!

lib/Target/Sparc/SparcFrameLowering.cpp
232–235 ↗(On Diff #25709)

As far as I can tell, I0_I1 (etc) don't ever get used bits set on them, so they don't need to be unmarked either.

245 ↗(On Diff #25709)

This code is indeed *not* correct. Thanks for catching that!! Any idea how to cover that in a test case?

lib/Target/Sparc/SparcISelLowering.cpp
2641–2642 ↗(On Diff #25709)

Not quite the only place, see also LowerSTORE, which has a similar piece of code.

Note the call to LowerLOAD from LowerOperation: the code there used to say "return LowerF128Load(Op, DAG);", which simply assumed that the type passed in was *always* MVT::f128 when called with ISD::LOAD. That's no longer true, because MVT::i64 is also marked for Custom handling via setOperationAction, so LowerOperation could be called with i64, now too.

Unlike store, I don't actually need any custom handling for it here (because the custom handling for it occurs in ReplaceNodeResults, instead), but this code does need to not do the f128 stuff for it.

t.p.northover added inline comments.May 18 2015, 9:55 AM
lib/Target/Sparc/SparcFrameLowering.cpp
232–235 ↗(On Diff #25709)

Ah, that makes sense. Their fundamental Units are already covered in the outer loop.

245 ↗(On Diff #25709)

Not sure. It might be visible in "llc -debug", but I'm not sure we encourage tests like that (it would certainly need a "REQUIRES: asserts" line).

lib/Target/Sparc/SparcISelLowering.cpp
2641–2642 ↗(On Diff #25709)

Ah yes, I completely missed the 2-line LowerF128 directly in the switch. Sorry about that.

jyknight updated this revision to Diff 25997.May 18 2015, 12:26 PM
jyknight retitled this revision from Sparc: Implement i64 load/store support for 32-bit sparc. to Sparc: Implement i64 load/store support for 32-bit sparc..
jyknight updated this object.

Minor tweaks as requested. Also included a new test file I'd neglected
to include last time (reserved-regs.ll)

Note that there's still an issue with this: it breaks calling a function with an v2i32 type, because I didn't add calling convention support for it -- even though I want it to have the default behavior, were that not a legal type, it still needs explicit handling. (That was covered by a test case only run against the default target)

I also plan to fix the core inlineasm code, so that the SelectInlineAsm hack can be deleted.

echristo accepted this revision.Jul 7 2015, 11:27 AM
echristo edited edge metadata.

Think you need to clang format bits, but otherwise looks ok. Request for a comment inline.

Thanks!

-eric

lib/Target/Sparc/SparcInstrInfo.cpp
299–304 ↗(On Diff #25997)

This could use a comment.

This revision is now accepted and ready to land.Jul 7 2015, 11:27 AM
jyknight updated this revision to Diff 31672.Aug 10 2015, 8:39 AM
jyknight edited edge metadata.

Update to trunk, fix CodeGen/Generic/vector.ll test failure by
implementing v2i32 calling convention.

This revision was automatically updated to reflect the committed changes.