This is an archive of the discontinued LLVM Phabricator instance.

[CodeGen][mips] Support `long_call/far/near` attributes
ClosedPublic

Authored by atanasyan on Jul 17 2017, 5:28 AM.

Details

Summary

This patch adds support for the long_call, far, and near attributes for MIPS targets. The long_call and far attributes are synonyms. All these attributes override -mlong-calls / -mno-long-calls command line options for particular function.

The main non-trivial part is the change in CodeGenModule::ConstructAttributeList routine. It is not enough to configure attributes in MIPSTargetCodeGenInfo::setTargetAttributes because this method applied to the function definitions only while we need to configure the attributes for function declarations as well.

Related backend patch is D35480.

Diff Detail

Repository
rL LLVM

Event Timeline

atanasyan created this revision.Jul 17 2017, 5:28 AM
rjmccall added inline comments.Jul 17 2017, 3:48 PM
include/clang/Basic/Attr.td
1191 ↗(On Diff #106852)

Because this is used for all three attributes, I think you should call it something more general. Perhaps MipsCallStyle?

1195 ↗(On Diff #106852)

This is not the standard naming convention for accessors. I suggest isLongCall() and isNearCall().

include/clang/Basic/AttrDocs.td
1336 ↗(On Diff #106852)

I suggest the following wording:

Clang supports the `__attribute__((long_call)), __attribute__((far))`, and
`__attribute__((near))` attributes on MIPS targets. These attributes may only be
added to function declarations and change the code generated by the compiler when
directly calling the function. The `near` attribute allows calls to the function to
be made using the `jal` instruction, which requires the function to be defined in the
same 256MB segment as the caller. The `long_call and far` attributes are
synonyms and require the use of a different call sequence that works regardless of
the distance between the functions.

These attributes take priority over command line switches such as `-mlong-calls`.

lib/CodeGen/CGCall.cpp
1810 ↗(On Diff #106852)

You should really put this in TargetCodeGenInfo::setTargetAttributes. Please just add a ForDefinition_t argument to that function and SetFunctionAttributes, then call setTargetAttributes from SetFunctionAttributes.

lib/Sema/SemaDeclAttr.cpp
5955 ↗(On Diff #106852)

You need to check for conflicts between the different attributes, and please add a test for that.

atanasyan updated this revision to Diff 107193.Jul 18 2017, 3:38 PM

Addressed review comment:

  • Split MipsLongCall into a couple of attributes MipsLongCall and MipsShortCall.
  • Change the documentation wording.
  • Keep the attributes handling in the setTargetAttributes.
  • Show error in case of combining incompatible attributes.
rjmccall edited edge metadata.Jul 18 2017, 4:14 PM

One minor revision, but otherwise looks great, thank you.

lib/CodeGen/CodeGenModule.cpp
1166 ↗(On Diff #107193)

I think you should probably pass IsForDefinition to setTargetAttributes. Targets may want to only set certain attributes on function definitions.

  • Pass IsForDefinition argument to the setTargetAttributes method.
rjmccall added inline comments.Jul 18 2017, 11:18 PM
lib/CodeGen/TargetInfo.cpp
1910 ↗(On Diff #107246)

To preserve existing behavior, please make all of the existing implementations you've modified just do an early return if !IsForDefinition.

In the MIPS implementation, you can just handle the long-call attributes first and then exit, unless you feel up to auditing the rest of the attributes there to see if they belong on declarations.

atanasyan updated this revision to Diff 107281.Jul 19 2017, 4:30 AM
  • Early return from setTargetAttributes methods if IsForDefinition is not true in all cases except handling MIPS "call style" attributes.

A few more minor tweaks.

lib/CodeGen/TargetInfo.cpp
2357 ↗(On Diff #107281)

That function has its own early exit, so do the early exit after calling it, please.

5538 ↗(On Diff #107281)

Same thing here.

6643 ↗(On Diff #107281)

"meaning" is a better word here.

sdardis edited edge metadata.Jul 19 2017, 10:25 AM

Some comments on the documentation.

include/clang/Basic/AttrDocs.td
1336 ↗(On Diff #106852)

requires the function to be defined

I'd change the "defined" to be "located".

in the same 256MB segment as the caller.

I'd change this to: "in the same naturally aligned 256MB segment as the caller."

This also needs a note saying that it has no effect for code compiled with -fpic.

rjmccall added inline comments.Jul 19 2017, 12:11 PM
include/clang/Basic/AttrDocs.td
1336 ↗(On Diff #106852)

Oh, yes, if these statements are true then it's absolutely important to include them. I was going off of the raw ISA specification, since jal just takes a relative offset, but if the toolchain has more specific requirements then those are what should be documented.

This whole feature is strange to me; do MIPS linkers just never introduce things like branch islands and lazy-binding functions?

atanasyan updated this revision to Diff 107381.Jul 19 2017, 2:26 PM
  • New wording of attributes documentation and comments.
  • Remove redundant if-return statements.
sdardis added inline comments.Jul 19 2017, 2:32 PM
include/clang/Basic/AttrDocs.td
1336 ↗(On Diff #106852)

since jal just takes a relative offset,

jal doesn't take an offset, the instruction takes an 26-bit immediate (called the instruction index) shifts it left 2 bits and combines it with the remaining upper bits of the address of the instruction in the delay slot of jal to form the new $pc--so it's possible to jump from the bottom of a 256MB segment to the top.

(My apologies for the long answer here, jal is one of the odder control transfer instruction for MIPS.)

This whole feature is strange to me; do MIPS linkers just never introduce things like branch islands and lazy-binding functions?

This feature is orthogonal to lazy-binding functions. The usage of 'long-call' and 'near' is to support bare-metal / static relocation model environments where the caller and callee might be in different memory segments (e.g. KSEG0 and KSEG1) or have a custom memory layout for their program's sections.

To the best of my knowledge, the GNU linker for MIPS only inserts stub functions / branch islands when compiling static code that calls PIC code.

rjmccall added inline comments.Jul 19 2017, 3:23 PM
include/clang/Basic/AttrDocs.td
1336 ↗(On Diff #106852)

Well, that's what I get for skimming the first search hit instead of taking the time to read the real documentation. Thank you for the correction.

Oh, and of course that's why you can't really use JAL in PIC: the linker could statically resolve instruction indexes within the current image, but only if the image gets loaded at a base address that's a multiple of 256MB! That's obviously too coarse-grained to be workable.

What an unfortunate instruction design.

rjmccall added inline comments.Jul 19 2017, 3:37 PM
lib/CodeGen/TargetInfo.cpp
2356 ↗(On Diff #107381)

No, sorry, I must not have been clear. We still need a check here, but we should do it after the first call.

The idea is that we want to give the generic x86-32 target an opportunity to add target attributes, whether this is a declaration or a definition. , Having done that, we come back to this function to add any Windows-specific target attributes. Since all the Windows-specific attributes are definition-only, we can just exit if it's only for a declaration.

5534 ↗(On Diff #107381)

Same thing here: please add a check, but only after you've given the generic ARM target an opportunity to set attributes in all cases.

My bad. I did not read your comment thoroughly.

  • Restore IsForDefinition checkings in WinX86_32TargetCodeGenInfo::setTargetAttributes and WindowsARMTargetCodeGenInfo::setTargetAttributes methods.

Hmm. Could you invert those conditions so that they early-return, just for consistency? Sorry this is dragging out so long, and thanks for being so patient.

lib/CodeGen/TargetInfo.cpp
2401 ↗(On Diff #107446)

And this one should go after the call; I just missed it in the earlier reviews.

5535 ↗(On Diff #107446)

Here.

2357 ↗(On Diff #107281)

Here.

atanasyan updated this revision to Diff 107461.Jul 20 2017, 1:28 AM

Addressed review comments.

rjmccall accepted this revision.Jul 20 2017, 8:58 AM

Thanks, that looks great!

This revision is now accepted and ready to land.Jul 20 2017, 8:58 AM
This revision was automatically updated to reflect the committed changes.