|
From: <sv...@va...> - 2012-06-15 20:55:53
|
florian 2012-06-15 21:55:43 +0100 (Fri, 15 Jun 2012)
New Revision: 2385
Log:
Fix a few issues as reported by the BEAM tool.
Patch by Carl Love (ce...@li...).
Modified files:
trunk/priv/host_ppc_defs.c
trunk/priv/host_ppc_defs.h
trunk/priv/host_ppc_isel.c
Modified: trunk/priv/host_ppc_defs.h (+2 -2)
===================================================================
--- trunk/priv/host_ppc_defs.h 2012-06-15 16:48:07 +01:00 (rev 2384)
+++ trunk/priv/host_ppc_defs.h 2012-06-15 21:55:43 +01:00 (rev 2385)
@@ -1036,9 +1036,9 @@
extern PPCInstr* PPCInstr_DfpRound ( HReg dst, HReg src, PPCRI* r_rmc );
extern PPCInstr* PPCInstr_DfpRound128 ( HReg dst_hi, HReg dst_lo, HReg src_hi,
HReg src_lo, PPCRI* r_rmc );
-extern PPCInstr* PPCInstr_DfpQuantize ( PPCAvFpOp op, HReg dst, HReg srcL,
+extern PPCInstr* PPCInstr_DfpQuantize ( PPCFpOp op, HReg dst, HReg srcL,
HReg srcR, PPCRI* rmc );
-extern PPCInstr* PPCInstr_DfpQuantize128 ( PPCAvFpOp op, HReg dst_hi,
+extern PPCInstr* PPCInstr_DfpQuantize128 ( PPCFpOp op, HReg dst_hi,
HReg dst_lo,
HReg src_hi,
HReg src_lo, PPCRI* rmc );
Modified: trunk/priv/host_ppc_defs.c (+3 -2)
===================================================================
--- trunk/priv/host_ppc_defs.c 2012-06-15 16:48:07 +01:00 (rev 2384)
+++ trunk/priv/host_ppc_defs.c 2012-06-15 21:55:43 +01:00 (rev 2385)
@@ -1092,7 +1092,7 @@
i->Pin.DfpRound128.r_rmc = r_rmc;
return i;
}
-PPCInstr* PPCInstr_DfpQuantize ( PPCAvFpOp op, HReg dst, HReg srcL, HReg srcR,
+PPCInstr* PPCInstr_DfpQuantize ( PPCFpOp op, HReg dst, HReg srcL, HReg srcR,
PPCRI* rmc ) {
PPCInstr* i = LibVEX_Alloc(sizeof(PPCInstr));
i->tag = Pin_DfpQuantize;
@@ -1103,7 +1103,7 @@
i->Pin.DfpQuantize.rmc = rmc;
return i;
}
-PPCInstr* PPCInstr_DfpQuantize128 ( PPCAvFpOp op, HReg dst_hi, HReg dst_lo,
+PPCInstr* PPCInstr_DfpQuantize128 ( PPCFpOp op, HReg dst_hi, HReg dst_lo,
HReg src_hi, HReg src_lo, PPCRI* rmc ) {
/* dst is used to pass left operand in and return result */
PPCInstr* i = LibVEX_Alloc(sizeof(PPCInstr));
@@ -2758,6 +2758,7 @@
mapReg(m, &i->Pin.DfpQuantize128.dst_lo);
mapReg(m, &i->Pin.DfpQuantize128.src_hi);
mapReg(m, &i->Pin.DfpQuantize128.src_lo);
+ return;
case Pin_DfpD128toD64:
mapReg(m, &i->Pin.DfpD128toD64.src_hi);
mapReg(m, &i->Pin.DfpD128toD64.src_lo);
Modified: trunk/priv/host_ppc_isel.c (+2 -2)
===================================================================
--- trunk/priv/host_ppc_isel.c 2012-06-15 16:48:07 +01:00 (rev 2384)
+++ trunk/priv/host_ppc_isel.c 2012-06-15 21:55:43 +01:00 (rev 2385)
@@ -1498,8 +1498,8 @@
return r_dst;
}
- if ((e->Iex.Binop.op == Iop_CmpF64) |
- (e->Iex.Binop.op == Iop_CmpD64) |
+ if ((e->Iex.Binop.op == Iop_CmpF64) ||
+ (e->Iex.Binop.op == Iop_CmpD64) ||
(e->Iex.Binop.op == Iop_CmpD128)) {
HReg fr_srcL;
HReg fr_srcL_lo;
|
|
From: John R. <jr...@bi...> - 2012-06-15 21:42:39
|
> New Revision: 2385
>
> Log:
> Fix a few issues as reported by the BEAM tool.
> Patch by Carl Love (ce...@li...).
> Modified: trunk/priv/host_ppc_isel.c (+2 -2)
> ===================================================================
> --- trunk/priv/host_ppc_isel.c 2012-06-15 16:48:07 +01:00 (rev 2384)
> +++ trunk/priv/host_ppc_isel.c 2012-06-15 21:55:43 +01:00 (rev 2385)
> @@ -1498,8 +1498,8 @@
> return r_dst;
> }
>
> - if ((e->Iex.Binop.op == Iop_CmpF64) |
> - (e->Iex.Binop.op == Iop_CmpD64) |
> + if ((e->Iex.Binop.op == Iop_CmpF64) ||
> + (e->Iex.Binop.op == Iop_CmpD64) ||
> (e->Iex.Binop.op == Iop_CmpD128)) {
This change produces equivalent-but-slower code on i686, x86_64,
PowerPC, ARM, and any other architecture that can perform multiple
tests without multiple branches (and thus avoid multiple
pipeline stalls due to multiple missed branch predictions.)
[x86: SETcc + bool; PowerPC: crAND, crOR etc; ARM: conditional
execution (including if-then)]
If BEAM insists, then BEAM needs to re-consider.
--
|
|
From: Florian K. <br...@ac...> - 2012-06-15 22:28:08
|
On 06/15/2012 11:43 PM, John Reiser wrote:
>>
>> - if ((e->Iex.Binop.op == Iop_CmpF64) |
>> - (e->Iex.Binop.op == Iop_CmpD64) |
>> + if ((e->Iex.Binop.op == Iop_CmpF64) ||
>> + (e->Iex.Binop.op == Iop_CmpD64) ||
>> (e->Iex.Binop.op == Iop_CmpD128)) {
>
> This change produces equivalent-but-slower code on i686, x86_64,
> PowerPC, ARM, and any other architecture that can perform multiple
> tests without multiple branches (and thus avoid multiple
> pipeline stalls due to multiple missed branch predictions.)
> [x86: SETcc + bool; PowerPC: crAND, crOR etc; ARM: conditional
> execution (including if-then)]
In this case the compiler could replace || with | as an optimization as
all subexpressions are free of side effects.
> If BEAM insists, then BEAM needs to re-consider.
BEAM is not insisting on anything. We could easily switch that complaint
off if we wanted to. But I don't think we want to do that.
Mixing boolean and bitwise expressions is a known source of trouble. If
you look around VEX code, || is used across the board when boolean
expressions are combined. So we should be consistent with existing practice.
Florian
|
|
From: Julian S. <js...@ac...> - 2012-06-15 22:40:09
|
On Saturday, June 16, 2012, Florian Krohm wrote:
> On 06/15/2012 11:43 PM, John Reiser wrote:
> >> - if ((e->Iex.Binop.op == Iop_CmpF64) |
> >> - (e->Iex.Binop.op == Iop_CmpD64) |
> >> + if ((e->Iex.Binop.op == Iop_CmpF64) ||
> >> + (e->Iex.Binop.op == Iop_CmpD64) ||
> >>
> >> (e->Iex.Binop.op == Iop_CmpD128)) {
> BEAM is not insisting on anything. We could easily switch that complaint
> off if we wanted to. But I don't think we want to do that.
> Mixing boolean and bitwise expressions is a known source of trouble. If
> you look around VEX code, || is used across the board when boolean
> expressions are combined. So we should be consistent with existing
> practice.
Yeah. Besides, this is in an instruction selector, which is hardly a
hot path. Register allocation is expensive, but insn selection barely
comes on the scale. I'd go for the simple version until such time
as we have profile data saying that insn selection is a bottleneck.
J
|