This is an archive of the discontinued LLVM Phabricator instance.

[MSP430] Add SRet support to MSP430 target
ClosedPublic

Authored by awygle on Jan 23 2017, 7:22 PM.

Details

Summary

This patch adds support for struct return values to the MSP430 target backend. It also reverses the order of argument and return registers in the calling convention to bring it into closer alignment with the published EABI from TI.

Diff Detail

Event Timeline

awygle created this revision.Jan 23 2017, 7:22 PM
pftbest added inline comments.
test/CodeGen/MSP430/cc_args.ll
55

Third argument should be back-filled into r13 register, as per section 3.3.4 of EABI.

awygle updated this revision to Diff 85630.Jan 24 2017, 2:15 PM

Updated patch to correctly backfill registers when "holes" are created by 64-bit arguments (EABI section 3.3.4) and to split 32-bit arguments when only one free register remains (EABI section 3.3.3)

This change looks good, but I don't have any rights to accept it, sorry. We'll have to wait for aKor to review it, you can try to ask him on irc if you see him.

asl added inline comments.Feb 4 2017, 1:36 AM
lib/Target/MSP430/MSP430ISelLowering.cpp
248

I'm a bit confused by logic here. Is there any reason why we shouldn't simply count from 0 here? Could also switch to range-for while here?

249–252

Minor nit: I'd prefer += 1 here.

324

Minor nit: I'd prefer -= 1 here

328

Please follow LLVM's coding style : no newline before "else if" / "else"

569

No braces

awygle updated this revision to Diff 87096.Feb 4 2017, 9:39 AM

Updated the diff to address the comments made by @asl .

Sorry for being pedantic, but your new diff adds a lot of trailing spaces to the code.

awygle updated this revision to Diff 87099.Feb 4 2017, 10:20 AM

Updated diff to hopefully address @pftbest's concerns about whitespace - I wasn't totally sure what you were pointing to but there's now no trailing whitespace in any of the code files that are modified by this patch.

Whitespace is gone, thank you.
Also I did some testing: compiled llvm with expensive checks on and address sanitizer, found no issues.
Compiled a non-trivial program and ran it on CC430 MCU, it worked fine.
So this patch LGTM

asl edited edge metadata.Feb 4 2017, 1:22 PM

Looks like one comment in ParseFunctionArgs was not addressed.

As I see it, CurrentArgIndex is overloaded with two (or more) tasks, so the code is confusing. Maybe it would be better if CurrentArgIndex was just a counter, and the other conditions can be handled by introducing boolean flags, for example.

awygle updated this revision to Diff 87118.Feb 4 2017, 3:18 PM

Updated patch to stop being so clever with initialization in ParseFunctionArgs

awygle added a comment.Feb 4 2017, 3:20 PM

The problem with ParseFunctionArgs was that it was being too clever with initializing CurrentArgIndex. It used to initialize to UINT16_MAX because that was "one less than" the first Arg's OrigArgIndex, but now that we have srets we can get NoArgIndex which happens to be UINT16_MAX. I originally tried to preserve the structure of the loop but now it just explicitly initializes instead of being clever.

This new revision is not good. It crashes on CodeGen/MSP430/vararg.ll test.

Sorry, not the vararg test, but this 3 tests:
CodeGen/MSP430/2009-08-25-DynamicStackAlloc.ll
CodeGen/MSP430/2009-12-22-InlineAsm.ll
CodeGen/MSP430/cc_ret.ll

asl added inline comments.Feb 4 2017, 4:21 PM
lib/Target/MSP430/MSP430ISelLowering.cpp
249–252

Is there any guard preventing calling this function when we're having zero args?

awygle updated this revision to Diff 87123.Feb 4 2017, 4:24 PM

Somehow managed to run check-all against an old version. Added obviously-required check for an empty vector. All tests now pass (and don't segfault).

I can confirm that now it's passing the tests

Are there any further corrections that need to be made before this can be merged?

asl accepted this revision.Feb 12 2017, 12:55 AM

LGTM with small change aboce

lib/Target/MSP430/MSP430ISelLowering.cpp
250

Simple early return here.

This revision is now accepted and ready to land.Feb 12 2017, 12:55 AM
awygle updated this revision to Diff 88119.Feb 12 2017, 3:56 AM

Great. Thank you. I've made the requested change.

@asl can you please commit this change?

Looks like D29916 broke this patch. Also CanLowerReturn should be marked with override to prevent compile warnings.

awygle updated this revision to Diff 90364.Mar 2 2017, 11:33 AM

Updated to work with latest SVN.

pftbest accepted this revision.Mar 2 2017, 12:28 PM

Looks good, Thank you.

This revision was automatically updated to reflect the committed changes.