https://bugs.kde.org/show_bug.cgi?id=436411
Julian Seward <jsew...@acm.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |jsew...@acm.org
--- Comment #3 from Julian Seward <jsew...@acm.org> ---
This is for the most part good, and apart from the following I have no
concerns.
I see two issues of concern, both pertaining to CPU feature-gating. The first
is in the front end:
+ if (bitU == 1 && size == X11 && opcode == BITS5(0,0,0,1,0)) {
+ /* -------- 1,11,00010 FABD h_h_h -------- */
..
and the same for the other added instructions: these need to be feature-gated,
so that they are not accepted on non-8.2 capable hosts. Consider what would
happen if, with these changes, we tried to run a binary containing these insns
on an 8.0 host. Then the front end will accept the insns, generate (eg
Iop_Add16F), which the back end will translate back into an 8.2 insn, and we
will take a SIGILL in the generated code. But that's not the correct
behaviour: what is required is that the front end declines to decode the insn,
and that in turn will cause the existing front end machinery to eventually
deliver a SIGILL to the guest.
The second area of concern is similar but in the back end, regarding the
Iex_Const case for iselIntExpr_RIL_wrk. To be honest I'm not sure that this
should have been changed at all -- unless the RIL field, with its division into
N/R/S fields, has acquired a new interpretation, but not a new layout, in 8.2
instructions. Can you clarify: does the existing immediate-for-bitfield
instruction-fragment (here called RIL, and the the ARM/ARM described using the
N/R/S triple) get a new interpretation in 8.2 instructions?
If it doesn't, and is instead a new kind of immediate field for these new
insns, then it seems to me that we'd need a new struct altogether, in similar
style to, but different from, ARM64RI6/ARM64RIL/ARM64RIA/ARM64AMode.
Independent of all of that, there's still the issues that iselIntExpr_RIL_wrk
can't be allowed to have different behaviour than it does now, without first
testing that we're on an 8.2 host rather than an 8.0 host.
In the backend, the top level iselSB_ARM64 passes around a `hwcaps_host`, and
that should contain whatever gating info is needed. In the front end,
disInstr_ARM64 similarly passes around an `archinfo` and so the
`archinfo->hwcaps` field will contain whatever's needed for gating in the front
end.
+ /* This is a temporary alert while work is done on adding v8.2
+ * support. It will be removed and fixed when Ity_I16 immediate
+ * encodings are understood better as v8.2 work progresses.
+ */
+ vpanic("iselIntExpr_RIL_wrk: TODO: unhandled Ity_I16 for Iex_Const");
Regardless of all of the above, that can't land .. it would potentially
destabilise the back end. If you don't want to handle the case specially, then
jump to the "default case" bit at the end of the function, which can handle
anything. (The same scheme applies to many other isel-functions too).
--
You are receiving this mail because:
You are watching all bug changes.
