Given a patch like D129506, using instructions not valid for the current target feature set becomes an error. This means that emitting Arm instructions in a Thumb target (or vice versa) becomes an error. As far as I can tell when running in Thumb mode only thumb thunks will be needed though, and in Arm mode only arm thunks are needed. This patch limits the emitted thunks to just the ones valid for the current architecture.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
The old version of this was only inserting one set of thunks even if the file contained multiple subtargets. This now uses InsertedThunks as a bitvector to distinguish between Thumb and Arm thunks being added. It should still only add each thunk once, but won't add both sets (arm/thumb) if they are not required.
Ping - it would be good to get this in, it will allow us enables more testing for the valid instructions in the Arm backend. Thanks
I can't claim to know what these thunks are doing in the first place, so just some general comments.
llvm/include/llvm/CodeGen/IndirectThunks.h | ||
---|---|---|
29–33 | Can we document what this contains? Now that it's more than did or did not insert something. | |
98 | inserted | |
llvm/lib/Target/ARM/ARMSLSHardening.cpp | ||
179 | Magic number detector says why are we looking for numbers instead of named values? Off the top of my head - because there's no good place to store such an enum/define that can be used in all the places it's needed. | |
llvm/test/CodeGen/ARM/speculation-hardening-sls-boththunks.ll | ||
4 | Thumb | |
5 | Is there reason to check that with only one of arm and thumb you only get thunks for that one? |
My apologies for missing this outstanding review for so long.
llvm/include/llvm/CodeGen/IndirectThunks.h | ||
---|---|---|
29–33 | This ThunkInserter is a base class combining the shared logic needed to insert thunks for a few more specific transformation passes. Exactly what needs to be tracked for "InsertedThunks" with this patch starts to depend on the more specific transformation pass. IIUC, with this pass what needs to be tracked is:
Given that what needs to be tracked is becoming depending on the class deriving from this base class, I wonder if it wouldn't be better to get the type needed for InsertedThunks from the Derived class? |
Thanks for the suggestions. This now changes the type to be a parameter passed to the base class, which can be an enum for Arm so long as we can still treat it like a bitmask.
llvm/test/CodeGen/ARM/speculation-hardening-sls-boththunks.ll | ||
---|---|---|
5 | The idea is that we should be checking that if we emitting instructions which require a target feature, that target feature should be present. Think of things like accidentally emitting NEON instructions when compiling for MVE, or emitting SVE2 instructions when you only have SVE. So we can't emit Thumb instructions if the target is for Arm. (Without constructing a subtarget with Thumb present). If you search for "verifyInstructionPredicates", you will see that most architectures have this check enabled, but Arm has always been missing. That is what I am attempting to fix. Improving how much we test emitted instruction features by enabling the option for Arm. In this case, as far as I understand, If we are in a Arm context then we will only need arm thunks. And Thumb will only ever use thumb thunks. This tests makes sure that in a module with both, we still end up with both sets of thunks. |
I just have a few more very minor nit-picky comments. Feel free to disagree with them.
I'm happy for this to go in.
I haven't looked in detail into the comments @DavidSpickett mad , so please also explicitly get his approval before committing.
Thank you!
llvm/include/llvm/CodeGen/IndirectThunks.h | ||
---|---|---|
24–25 | I see later in the code that InsertedThunksTy must support operator |= and that the semantics of the |= operation must be somewhat like doing a union/max kind-of operation. | |
29–34 | Extreme nit-pick: I'd add "so far" to the end of the sentence. | |
llvm/lib/Target/ARM/ARMSLSHardening.cpp | ||
164 | Maybe InsertedThunkKind would be a little bit more descriptive than ArmInsertedThunks for what this enum is modelling? | |
174–175 | Could it be that this has not been clang-formatted? |
I see later in the code that InsertedThunksTy must support operator |= and that the semantics of the |= operation must be somewhat like doing a union/max kind-of operation.
Not sure if it would be worthwhile to document that at this location, or if there is a way in modern C++ to somehow enforce that.
Probably best action is not to change anything in the patch - I thought I'd just share my thought in case there is anything about it that can be improved in this patch.