Clearly specify the instruction_set effects

[Lokathor]

This updates the instruction_set attribute entry to more clearly explain the additional restrictions that the attribute places on inlining. Since Rust performance heavily relies on inlining, it's actually a fairly big deal to document it.

Pinging @pnkfelix as the liaison for the instruction_set attribute during its development.

This explanation is actually slightly looser than the explanation given in the RFC: this explanation allows inlining between no-attribute code without inline asm and other code. So, T-lang should likely approve the update with an FCP or similar process. The intent is that once this is approved as the documentation for how the attribute works then rust-lang/rust#104121 (which updates the MIR inliner to match these rules) will be easier to land.

Updates the attributes page to specify what the instruction_set attribute does and where you can use it.

[pnkfelix]

Nominating for a quick skim from T-lang. In particular, the rules around inlining, starting here:

https://github.com/rust-lang/reference/pull/1307/files#diff-11f03f772eabdb1a762b90f1ce58afd34c7938b20beae226e451da44d4fcd73aR378

are we okay with this set of rules? There are three basic rules there, the first two dictating the cases where inlining will be disallowed, and the third dictating when inlining will be allowed (and thus the calling function must not rely on details of the instruction set, or at least not details that would be in conflict with the instruction set of the callee that is being inlined).

The two specific questions are:

1. Are we all okay with the first two rules, in terms of saying "these are cases where it is safe to rely on the instruction set"?
   • (I personally think they seem to be very clear and intuitive rules, a kind of case where I expect people to say "well yes, how could it be any other way?")
2. Are we okay with the third rule, in terms of saying "in all other cases the code of a fn must not rely on the (implicit) instruction set"?
   • (I personally am fine with it, and its hard for me to imagine a scenario where we would want to allow a fn to have such an implicit reliance without any marker via the instruction_set attribute. But, having said that ... I assume it would then be a source of potential undefined behavior, so we should still take care in making a decision here.)

[Lokathor]

I should perhaps note that if a function is inlined in some places as well as being fully generated to call over to from other places, the generated version will be of the target's default instruction set as one would expect.

The goal here is to carve out a little wiggle room for the MIR inliner to do more work. Without the ability to inline no-attribute code into attribute'd code it makes any use of the attribute a complete drop in performance.

For more background on how we got here you can also read the zulip topic: https://rust-lang.zulipchat.com/#narrow/stream/189540-t-compiler.2Fwg-mir-opt/topic/What.20exactly.20does.20the.20MIR.20optimizer.20do.3F

[pnkfelix]

Hey @Lokathor , sorry for the long delay in response here

The T-lang design team talked about this PR in its triage meeting yesterday

No one at the meeting expressed any problem with the rules as stated.

There was a slight concern about whether the language here is potentially misleading: we already don't say what inlining exactly "means", and all discussions of inlining attributes repeatedly say that they are all merely hints, with no requirements that inlining actually happen, or actually be strictly prohibited.

So, there was some feedback about whether this text could try incorporate that notion (redundantly, I acknowledge).

In other words, we wish for it to be clear that this text is non-normative, and is just meant to set users expectations about what might happen, without making strict guarantees in one direction or the other.

[pnkfelix]

Also, there was some followup discussion on zulip

in that conversation, while attempting to summarize the meaning of the third rule, I wrote the something like the following:

If your function has neither the instruction_set attribute nor inline assembly, then the code you write within that function should not presume any particular instruction set

and there was some feedback indicating that this is a better way to write the third rule in question, with the belief that the resulting effects on whether inlining is allowed or not falls out naturally from that phrasing.

Maybe the third rule can instead be pulled out and phrased in something like the above form, and then you can keep the discussion of inlining, including all three parts, but reframe it all as consequences of the new phrasing?

[nikomatsakis]

@rustbot labels -I-lang-nominated

Removing nomination as this is waiting on @Lokathor

[Lokathor]

Ah, slipped my mind, I'll try to get to this soon

[Lokathor]

I think the new wording is sufficiently improved?

[ehuss]

Just some editorial nits.

[ehuss]

@pnkfelix Did you have any comments on the updated text? Do you want to start an FCP to formally approve the changes, or was the team discussion sufficient?

[nikomatsakis]

@rustbot labels +I-lang-nominated

Raising for team discussion. I think that this text is fine as is. This whole feature is on that "grey line" where it doesn't theoretically affect language semantics etc etc.

[joshtriplett]

Suggested change

	If your function has neither the `instruction_set` attribute nor inline assembly, then the code you write within that function should not presume any particular instruction set.
	If your function has neither the `instruction_set` attribute nor inline assembly, and you're compiling for a target with multiple instruction sets, the code you write within that function should not presume any particular instruction set.

[joshtriplett]

This seems like it's specific to ARM and Thumb; this may not necessarily universally be true for all future uses of instruction_set.

[joshtriplett]

Suggested change

	This ends up creating a limitation to how often code is inlined:
	For ARM in particular, `instruction_set` adds a limitation to where code gets inlined:

[Amanieu]

I don't think the reference should make any mention of inlining, since that is an implementation detail. Instead the text should clearly document the actual guaranteed effects of #[instruction_set]:

• If the address of the function is taken as a function pointer. The low bit will be set to 0 or 1 depending on the instruction set.
• Any inline assembly in the function will use the specified instruction set instead of the target default.

The restrictions on inlining are mainly an artifact of how inline assembly is compiled with LLVM: it uses the instruction set of the containing function after ininling. As such we need to ensure that any functions containing inline assembly are not inlined into functions with a different instruction set.

[Lokathor]

@joshtriplett updated per the 2023-06-27 T-lang meeting, ready for FCP if the text seems fine.

[joshtriplett]

@rfcbot merge

[rfcbot]

Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged team members:

• @joshtriplett
• @nikomatsakis
• @pnkfelix
• @scottmcm
• @tmandry

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

[RalfJung]

The PR description and title still need to be updated to the new content.

[Lokathor]

should be fixed

[nikomatsakis]

@rfcbot reviewed

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

psst @joshtriplett, I wasn't able to add the final-comment-period label, please do so.

[nikomatsakis]

@rustbot labels -I-lang-nominated +final-comment-period

[rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

psst @joshtriplett, I wasn't able to add the finished-final-comment-period label, please do so.

[ehuss]

Thanks everyone!