i#6499: AArch64 codec: mark instructions with governing predicate as predicated #6535
Conversation
…predicated This patch adds the DR_PRED_GOVERNING flag to instructions containing any governing predicate registers as well as tagging the governing register with DR_OPND_IS_GOVERNING. To do this, it extends the codec functionalty such that operands may have "flags" which allow us to add some special code generation when they are encountered. In this case we ammend the instr decode/encode functions to both tag the governing predicate and the instr itself. issue: #i6499 Change-Id: If93aedb4036e7ec18b20bb6f56f4297b433737b9
Change-Id: I3f6e16f057b2fa56d4d480a6c48b3b4734635b2e
| bool | ||
| opnd_is_governing(opnd_t op) | ||
| { | ||
| return opnd_is_predicate_reg(op) && ((op.aux.flags & DR_OPND_IS_GOVERNING) != 0); |
There was a problem hiding this comment.
style: DR code uses TEST (==TESTANY) or TESTALL for all bit checks
There was a problem hiding this comment.
Because these can be used for clients, I believe TEST is undefined. The other test macros in this file all appear to be behind a ifded DYNAMORIO_INTERNAL
| @@ -175,6 +175,13 @@ opnd_is_predicate_zero(opnd_t op) | |||
| return opnd_is_predicate_reg(op) && ((op.aux.flags & DR_OPND_IS_ZERO_PREDICATE) != 0); | |||
There was a problem hiding this comment.
style: DR code uses TEST (==TESTANY) or TESTALL for all bit checks
core/ir/instr_api.h
Outdated
| @@ -139,6 +139,9 @@ typedef enum _dr_pred_type_t { | |||
| DR_PRED_AL, /**< ARM condition: 1110 Always (unconditional) */ | |||
| # ifdef AARCH64 | |||
| DR_PRED_NV, /**< ARM condition: 1111 Never, meaning always */ | |||
| DR_PRED_GOVERNING, /** Used for AArch64 SVE instructions with governing predicate | |||
There was a problem hiding this comment.
Since the x86 mask concept seems to be the same (right? a bit per load/store element to enable/disable it?), let's try to share the interface as much as possible to make it easier on clients. So SVE calls it a "governing predicate register" while AVX-512 calls them "opmask registers". Is there some generic term we could use for the predicate so we can have the same DR_PRED_ name for both architectures so clients don't have to have ifdefs for the same functional code? "DR_PRED_MASKED" seems clearer to me but I'm not sure if that's just b/c of the x86 description: does "masked" makes sense on the aarch64 side? Even if we decided "DR_PRED_GOVERNING" was the right cross-arch name we'd want to update the dox comment to make it clear this is a cross-arch predicate concept.
There was a problem hiding this comment.
"DR_PRED_MASKED" seems clearer to me but I'm not sure if that's just b/c of the x86 description: does "masked" makes sense on the aarch64 side?
Short answer is that "masked" is too limited a name for AArch64 SVE predicate register behaviour.
Longer answer:
SVE has 3 flavours of governing predicate registers, two mask-like active/inactive and one control/selector (e.g. select which elements from source vector are acted upon by op):
<Pg>/Z as in e.g. BICS <Pd>.B, <Pg>/Z, <Pn>.B, <Pm>.B
<Pg>/M as in e.g. ABS <Zd>.<T>, <Pg>/M, <Zn>.<T>
<Pg> as in e.g. SPLICE <Zdn>.<T>, <Pg>, <Zdn>.<T>, <Zm>.<T>
Because of this, we went for DR_PRED_GOVERNING rather than DR_PRED_MASKED.
There was a problem hiding this comment.
@derekbruening are you happy with using DR_PRED_GOVERNING and updates to Doxygen comments to clarify the cross-architectural difference between the predicates?
Alternatively, we could stick to DR_PRED_MASKED if it keeps the code simpler, and highlight the difference in meaning between x86 and aarch64 in Doxygen comments.
There was a problem hiding this comment.
DR_PRED_GOVERNING can work: I don't think the term "governing" is used anywhere in the x86 ISA so it is just a generic term. (Note that for me "masked" could cover the "control/selector for which elements are acted upon" -- "masked" also seems a general term, and "which are acted upon" sounds very similar to "active/inactive".)
There was a problem hiding this comment.
We'll stick with DR_PRED_MASKED to keep naming consistent and simple.
There was a problem hiding this comment.
Latest patch changes all the DR_PRED_GOVERNING to DR_PRED_MASKED. This is currently still only defined for AArch64 as I assume the work to add it to x86 falls under #6510
Change-Id: I784c985847ecb900f680dd12c1561dc1bc2ea428
|
The x86 failures look like #6417. |
|
This PR broke our build with a python error: #6580 |
This patch adds the DR_PRED_MASKED flag to instructions containing any governing predicate registers as well as tagging the governing register with DR_OPND_IS_MASKED.
To do this, it extends the codec functionality such that operands may have "flags" which allow us to add some special code generation when they are encountered. In this case we amend the instr decode/encode functions to both tag the governing predicate and the instr itself.
issue: #6499