https://bugs.kde.org/show_bug.cgi?id=364948
--- Comment #4 from Julian Seward <jsew...@acm.org> ---
(In reply to Carl Love from comment #1)
> Created attachment 99775 [details]
> Patch 5 of 5 to add VEX support for Power ISA 3.0 instructions
I have a number of concerns here, but nothing that can't be relatively
easily fixed. They fall into 5 areas:
[1] naming and possible duplication of new IROps
[2] front end: duplication of IR trees (the IRExpr* vs IRTemp thing)
[3] front end: generation of excessively verbose IR for some vector insns
[4] misc other correctness comments/queries
[5] various typos in comments
To go through them in order:
------ [1] naming and possible duplication of new IROps ------
diff --git a/VEX/pub/libvex_ir.h b/VEX/pub/libvex_ir.h
+ Iop_MulAddF128, // (A * B) + C
+ Iop_MulSubF128, // (A * B) - C
+ Iop_NegMulAddF128, // -((A * B) + C)
+ Iop_NegMulSubF128, // -((A * B) + C)
Call these, respectively: MAddF128, MSubF128, NegMAddF128,
NegMSubF128, so as to be consistent with the 32/64 bit variants
+ Iop_ConvI64UtoF128, /* signed I64 -> F128 */
+ Iop_ConvI64StoF128, /* signed I64 -> F128 */
+ Iop_ConvF64toF128, /* F64 -> F128 */
+ Iop_ConvF128toF64, /* IRRoundingMode(I32) x F128 -> F64 */
+ Iop_ConvF128toF32, /* IRRoundingMode(I32) x F128 -> F32 */
Remove the leading Conv (eg Iop_I64UtoF128), so as to make them
consistent with other conversion operations.
+ /* Conversion signed 32-bit value to signed BCD 128-bit */
+ Iop_I128StoBCD128,
+
+ /* Conversion signed BCD 128-bit to signed 32-bit value */
+ Iop_BCD128toI128S,
The comments don't seem to match the names. Is the integer value
32 bits or 128 bits?
+ /* -- Vector to/from half conversion -- */
+ Iop_F16x4toF32x4, Iop_F32x4toF16x4, Iop_F64x2toF16x2, Iop_F16x2toF64x2,
You only need one lane specifier here, so as to be consistent with
other ops:
F16toF32x4, F32toF16x4, F64toF16x2, F16toF64x2
and in fact the first two already seem to exist. Are these different?
---- [2] front end: duplication of IR trees (the IRExpr* vs IRTemp thing) ----
+static void putC ( IRExpr* e )
+static IRExpr * create_FPCC( IRExpr *NaN, IRExpr *inf,
+ IRExpr *zero, IRExpr *norm,
+ IRExpr *dnorm, IRExpr *pos,
+ IRExpr *neg ) {
+static IRExpr * create_C( IRExpr *NaN, IRExpr *zero,
+ IRExpr *dnorm, IRExpr *pos,
+ IRExpr *neg ) {
These functions all use their input trees more than once and so will
duplicate them and the computations done by them. Please change them so
that the passed arguments are IRTemps instead.
There may be more such functions -- I am not sure if this is all of
them.
---- [3] front end:
generation of excessively verbose IR for some vector insns ----
+ case 28: // vctzb, Vector Count Trailing Zeros Byte
Too complex; use a vector IRop
+ case 29: // vctzh, Vector Count Trailing Zeros Halfword
Ditto
+ case 30: // vctzw, Vector Count Trailing Zeros Word
Ditto
+ case 31: // vctzd, Vector Count Trailing Zeros Halfword
Ditto
For the above cases (28/29/30/31), make yourself a set of vector IROps
to do this:
Iop_Ctz{8x16, 16x8, 32x4}
See existing ops Iop_Clz8x16, Iop_Clz16x8, Iop_Clz32x4 as an example
+ case 0: // bcdctsq. (Decimal Convert to Signed Quadword VX-form)
+ /* Check each of the nibbles for a valid digit 0 to 9 */
+ inv_flag[0] = newTemp( Ity_I1 );
+ assign( inv_flag[0], mkU1( 0 ) );
Didn't you do a C helper function for this in the previous patch?
This generates terribly verbose code.
------ [4] misc other correctness comments/queries ------
+static
+Bool FPU_rounding_mode_isOdd (IRExpr* mode) {
+ /* If the rounding mode is set to odd, the the expr must be a constant U8
+ * value equal to 8. Otherwise, it must be a bin op expressiong that
+ * calculates the value.
+ */
+
+ if (mode->tag != Iex_Const)
+ return False;
+
+ vassert(mode->Iex.Const.con->tag == Ico_U32);
+ if (mode->Iex.Const.con->Ico.U8 == 0x8)
+ return True;
+
+ vex_printf("ERROR: FPU_rounding_mode_isOdd(), constant not equal to
expected value\n");
+ return False;
+}
Doesn't seem right to me. What happens if mode isn't an Iex_Const?
Do you really want to just return False? Shouldn't the system assert
if that happens?
+++ b/memcheck/mc_translate.c
+#if !(defined(VGA_ppc64be) || defined(VGA_ppc64le))
tl_assert(ty != Ity_I128);
+#endif
Don't make this conditional
------ [5] various typos in comments ------
Some of these occur several times -- copy n pasted? Can you search
to find all dups?
+ /* 128-bit multipy by 10 instruction, result is lower 128-bits */
+ Iop_MulI128by10,
Nit: please fix typos (multipy) (in various places)
+++ b/memcheck/mc_translate.c
+ /* Widen 1st arg to I64. Since 1st arg is typically a rounding
That should say I128, not I64.
+++ b/VEX/priv/guest_ppc_toIR.c
@@ -1,4 +1,3 @@
-
Nit: please don't remove the initial blank line
/*------------------------------------------------------------*/
+void setup_value_check_args( IRType size, IRTemp *exp_mask, IRTemp *frac_mask,
Nit: blank line between comment and start of code
+/* The following functions check the floating point value to see if it
+ is zero, infinit, NaN, Normalized, Denormalized.
Nit: infinity
+static void generate_store_FPRF( IRType size, IRTemp src ) {
Nit: opening brace on its own line
+ /* Calcuulate the floating point result field FPRF */
Nit: Calculate
+++ b/VEX/pub/libvex_ir.h
+ Iop_NegMulAddF128, // -((A * B) + C)
+ Iop_NegMulSubF128, // -((A * B) + C)
The comment on the second one isn't right.
+ /* Note: PPC only coverts the 16-bt value in the upper word
Nit: typo, bt -> bit
+ * instrution set.
instrution
+ AltiVec 128 bit integer multiyply by 10 Instructions
multiyply
--
You are receiving this mail because:
You are watching all bug changes.
