Embedded Trace Extension and Trace Buffer Extension are optional future architecture extensions. (cf. https://developer.arm.com/architectures/cpu-architecture/a-profile/exploration-tools) Their system registers are documented here: https://developer.arm.com/docs/ddi0601/a ETE shares register names with ETM. One exception is the ETE TRCEXTINSELR0 register, which has the same encoding as the ETM TRCEXTINSELR register (but different semantics). This patch treats them as aliases: the assembler will accept both names, emitting identical encoding, and the disassembler will keep disassembling to TRCEXRINSELR.
Details
Details
Diff Detail
Diff Detail
- Repository
- rL LLVM
Event Timeline
Comment Actions
This needs a subtarget feature so that it isn't enabled for targets which don't have the extension.
| llvm/lib/Target/AArch64/AArch64SystemOperands.td | ||
|---|---|---|
| 1473 ↗ | (On Diff #206179) | I don't think these classes add anything, it would be better to stick to the pattern used elsewhere in this file. If you want to reduce the line lengths, we don't really need the comments with the register descriptions - all we care about in LLVM is the assembly name and the encoding. |
Comment Actions
This needs a subtarget feature so that it isn't enabled for targets which don't have the extension.
Both extensions are on purpose enabled unconditionally:
- ETE shares most of the system registers with ETM and these were historically unconditionally enabled in LLVM. It would be inconsistent to add a command line option, which enables just the few extra ETE registers, or it would break existing builds if we were to explicitly enable ETM registers.
- ETE requires TRBE, after unconditionally enabling ETE, it only makes sense to do the same for TRBE
Comment Actions
ETE shares most of the system registers with ETM and these were historically unconditionally enabled in LLVM.
I don't think this matters here - existing registers can stay as they are, and the new subtarget features should just cover new registers.
It would be inconsistent to add a command line option, which enables just the few extra ETE registers
I don't see why this would be inconsistent, we now add subtarget features for every other new optional architectural feature. Maybe we should add a feature for ETM if it is optional, but that doesn't affect this patch.
or it would break existing builds if we were to explicitly enable ETM registers.
Do you mean if we were to make ETM off by default? Agreed, we shouldn't do that.
Comment Actions
I don't think this matters here - existing registers can stay as they are, and the new subtarget features should just cover new registers.
It would be inconsistent to add a command line option, which enables just the few extra ETE registers
I don't see why this would be inconsistent, we now add subtarget features for every other new optional architectural feature. Maybe we should add a feature for ETM if it is optional, but that doesn't affect this patch.
ETE is not an extension over or a next version of ETM - it's a completely standalone extension that happens to share with ETM many register names and encodings, but not necessarily register semantics.
I don't think it's logical for any extension to have a mix of conditional/unconditional availability of registers - either all registers should be unconditionally available,
or all registers should be behind a target feature and a command line option. But making all of the ETE registers conditional on a target feature would affect that set of ETM registers that
have the same name and encoding, breaking compatibility.
or it would break existing builds if we were to explicitly enable ETM registers.
Do you mean if we were to make ETM off by default? Agreed, we shouldn't do that.
Yes, exactly, I meant "if we were to require explicitly enabling ETM registers".
| llvm/lib/Target/AArch64/AArch64.td | ||
|---|---|---|
| 743 ↗ | (On Diff #208648) | Why are you enabling this for the generic target? No existing hardware will have these instructions, so we should reject them unless they're explicitly enabled. |
| llvm/lib/Target/AArch64/AArch64.td | ||
|---|---|---|
| 743 ↗ | (On Diff #208648) | That's because we have no other means to enable them, thus they are enabled by default. Our plan is when the future architecture extensions find their place in some future architecture, then to have them enabled for that architecture only. Until then we want to make them available for people to experiment. |
| llvm/lib/Target/AArch64/AArch64.td | ||
|---|---|---|
| 743 ↗ | (On Diff #208648) | In which case we should add an actual way to turn this on (an extension flag in targetparser), rather than hijacking an existing architecture. |
Comment Actions
These are trace extensions that will be used by a niche group of developers. They add no instructions (only system registers) and they will not be generated by the compiler. Adding them by default does not have any side-effect, i.e. everyone will still see the same behavior in their code unless they start using these registers on purpose.
IMHO avoiding a command line option will give us a better implementation story going forward.
Without a command line option:
- Someone wants to use ETE & TRBE right now: just write code using the registers.
- The command line can be anything that compiles for aarch64, via the "generic" target.
- When there is a decision about where these extensions will land, we move them outside the generic target and put them where they really belong.
- This will break some people, but I think this is actually good. We don't want someone building with ETE & TRBE enabled in an armv8.2-a image if the extensions finally land in a newer armv8.x-a.
With a command line option:
- You have to enable the extension, which is an unnecessary step because you will not get anything out of it unless you explicitly use the registers.
- The only benefit would be an error message if you try to use the registers and build without the extension. However, at this point in time this is not too important (actually I would find it annoying): if someone is using the extension, it is because they really know what they are doing.
- When these extensions finally land in some architecture armv8.x-a, we may enable them by default there. However, someone using ETE & TRBE and building for armv8.2+ete+trbe will not see any error, will keep generating images that use these extensions, and these binaries may even be distributed to be run in armv8.2-a cores, which will obviously fail at runtime.
Ideally, the AArch64 back-end would have a HasV8_0aOps TargetFeature, and we would add ETE & TRBE there. Since adding HasV8_0aOps requires some proper refactoring, I think the generic processor is a good place to put them for now. Do you have any issue in mind that could come up if we add these extensions to the generic processor model?
Additionally, I personally like better the idea that users will know for sure (via test failures) when we change the minimum architecture for these features. Also we avoid adding yet another architecture extension that may be deprecated soon. Can you anticipate any problem if we don't add a command line option?
Comment Actions
Ok, but please add a comment, probably in AArch64.td, explaining this, and that turning it on by default is only temporary.