rustc_target: Begin unifying Arch enum

[workingjubilee]

There are a few different instances of this Arch enum in rustc_target. I would like to unify them so that we use shared code in more cases, as part of an incremental step towards turning Target into a proper builder with type-level correctness instead of its current constructor. However, it's definitely true the immediate changes look somewhat jank and have the disadvantage of in some cases handling some code less exhaustively, so I'm going to ask some maintainers if it's okay.

This PR currently starts with lifting from the Apple code, so

r? @madsmtm

[rustbot]

These commits modify compiler targets.
(See the Target Tier Policy.)

[workingjubilee]

also cc @thomcc

I intend to make the QNX targets part of this PR too, and also reuse this code elsewhere in spec, but I believe this will affect maintaining the code for the Apple targets more in practice.

[bors]

☔ The latest upstream changes (presumably #145126) made this pull request unmergeable. Please resolve the merge conflicts.

[madsmtm]

Sorry for the delay in reviewing this! Generally on-board with having a unified Arch enum.

@rustbot author

[madsmtm]

Hmm, I get why you'd want to simplify things in this manner (arch enum with subarch enums), but I'm not sure that's actually a good idea?

I particularly don't like the Arch::Arm64 to Aarch64Ver::V8A.into() change, it makes it harder to understand what's going on (are we specifying the arch or subarch here?), and it becomes incorrect as soon as you pass -Ctarget-cpu=apple-a10.

I'd prefer a single-level Arch enum, and rather have helpers like .is_aarch64() or .is_arm() (if those are even useful in the face of things like ARM64EC).

[workingjubilee]

It's always been my experience that "single enum with helpers" is worse on a few different levels. Namely, my goal is generally to incentivize matching against precisely the relevant patterns and to minimize the noise of other patterns. In particular, I believe the top level `match` should ideally not bottom out to a plain `_`. The problem is that such is effectively what `if let` does. If the only complaint is an `into`, I can work on reducing that. But if the problems are deeper, I am happy to think of a different approach if this does not suit. It's been a while so I'm not sure what you mean by `-Ctarget-cpu=apple-a10` changing things, though. Do the number of changes necessary actually alter significantly...?

[madsmtm]

It's always been my experience that "single enum with helpers" is worse on a few different levels. Namely, my goal is generally to incentivize matching against precisely the relevant patterns and to minimize the noise of other patterns. In particular, I believe the top level match should ideally not bottom out to a plain _. The problem is that such is effectively what if let does.

I went into this with the presumption that the plan is to use this Arch as Target.arch? And then maybe determine things like TargetOptions.cpu based on that and the OS?

If that is the case, then I kinda feel like we don't really have the data on whether a big or several smaller enums would be preferable? E.g. if you're gonna match on the subarch all the time anyhow, then it's just extra noise.

I did a quick count on the number of archs in rustc's target names, and got 76, which admittedly is fairly large, so then again, maybe I'm wrong here.

It's been a while so I'm not sure what you mean by -Ctarget-cpu=apple-a10 changing things, though. Do the number of changes necessary actually alter significantly...?

What I meant here is that I think Aarch64Ver::V8A is a misnomer, because options like the one I mentioned could change it such that the "V8A" part is no longer true. In that sense, Aarch64Ver::_64 would be preferable IMO (exactly because it is less precise, as it is not actually the target name that controls the subarch).

If the only complaint is an into, I can work on reducing that.

That would certainly help, Arch::Aarch64(Aarch64Ver::V8A) would be more understandable IMO.

[workingjubilee]

What I meant here is that I think Aarch64Ver::V8A is a misnomer, because options like the one I mentioned could change it such that the "V8A" part is no longer true. In that sense, Aarch64Ver::_64 would be preferable IMO (exactly because it is less precise, as it is not actually the target name that controls the subarch).

I simply don't understand what you mean. When reasoning about target architectures, the minimum is burned into the target. Later usage of -Ctarget-cpu does not and must not change that minimum, and this has important knock-on effects for how we handle compilation of targets.

[madsmtm]

When reasoning about target architectures, the minimum is burned into the target.

I guess that's fair.

---

In any case, this is somewhat outside my domain knowledge. Feel free to do whatever you think is best here, as long as we do the Aarch64Ver::V8A.into() -> Arch::Aarch64(Aarch64Ver::V8A) in the target definitions then I'm fine with it.

View changes since this review