|
From: Julian S. <js...@ac...> - 2013-01-04 11:22:03
|
2013 greetings to all.
I've spent a bit of time fine-tuning the IR machinery lately, in
order to try and do better on ARM and particularly Thumb code.
Due to the use of conditionalisation, ARM/Thumb stress the IR
optimisation machinery in weird ways that the other front ends
don't, and we are missing some obvious tricks.
One change I am looking at is changing the guard type of Mux0X
from I8 to I1. This makes it consistent with I1 guard types for
Dirty helpers and for Exits. The use of I8 as a guard comes from
very early experiments supporting x86 via IR. It seemed like a
good idea at the time, but makes no sense really.
Changing the order of the second and third args in Mux0X, so it
matches the familiar C ?-: syntax, and possibly renaming it,
would be a nice thing, but does not change the generated code.
If anyone wants to volunteer to do that (and verify the change is
correct :) please speak up.
------------
The key thing I want to do once that is finished is make the
constant folder able to look 'backwards' through expressions in
the flattened IR, when looking for transformations to do. This
should be easy since the folder now carries around a mapping from
IRTemps to IRExprs. Currently the folder only looks at "one level"
of expression, which means we miss out on opportunities to fold out
pointless identities such as
CmpNE32(1Uto32(boolexpr), 0) ==> boolexpr
and many others, particularly relating to Memcheck
instrumentation. Some such folding is done in the tree builder,
but that is right at the end of the compilation pipeline, so it
is too late for other transformations to benefit from it. Doing
it early would be so much better. Also, doing it just once in
iropt would avoid various half-backed hacks in the instruction
selectors which try to deal with the worst of it, on an ad hoc
architecture-specific basis.
In particular, ARM/Thumb makes it clear the folder needs to learn
about folding nested Mux0Xs. The following transformations would
be useful
Mux0X(c, Mux0X(c, x, y), z) ==> Mux0X(c, x, z)
Mux0X(c, x, Mux0X(c, y, z)) ==> Mux0X(c, x, z)
Mux0X(c, x, Mux0X(!c, y, z)) ==> Mux0X(c, x, y)
This last one is interesting because it appears a lot in idiomatic
Thumb code from gcc. To get either X or Y in a register, gcc
generates
cmp ... // set the flags
moveq rD, X // rD := X if Z flag set
movne rD, Y // rD := Y if Z flag clear
Currently we wind up with an IR expression of the form
Mux0X(c, X, Mux0X(!c, Y, old-value-of-rD))
Because iropt doesn't currently know that 'c || !c' covers all
possibilities, it keeps the old value of rD alive unnecessarily.
------------
Unrelated to all this .. I have also more or less finished a
first implementation of direct IR support for conditional loads
and stores, in the COMEM branch. This improves accuracy of
Memcheck on ARM and should facilitate support of AVX/AVX2
scatter/gather loads/stores. I expect to merge this to trunk in
the next couple of weeks or so. There is still a bit of
adjustment to all the other (non-ARM) backends that needs to be
done first.
J
|
|
From: Florian K. <br...@ac...> - 2013-01-06 14:48:22
|
On 01/04/2013 06:21 AM, Julian Seward wrote: > > One change I am looking at is changing the guard type of Mux0X > from I8 to I1. This makes it consistent with I1 guard types for > Dirty helpers and for Exits. Seconded. > Changing the order of the second and third args in Mux0X, so it > matches the familiar C ?-: syntax, and possibly renaming it, > would be a nice thing, but does not change the generated code. > If anyone wants to volunteer to do that (and verify the change is > correct :) please speak up. Yes this would be good to do. I never understood why this IROp was defined this way and I made a few mistakes using it at the time. The change is obviously mechanical. But I would not know how to verify it other than having somebody else proof-read the patch. Passing regtest is too weak a criterion... I'm willing to make the change if you and/or other port maintainers are willing to double-check. Florian |
|
From: Julian S. <js...@ac...> - 2013-01-06 15:48:19
|
> > One change I am looking at is changing the guard type of Mux0X > > from I8 to I1. This makes it consistent with I1 guard types for > > Dirty helpers and for Exits. > > Seconded. I have this working now for ARM, and, obviously, the generic IR stuff has been fixed up too. I can do the front/back end fixes for x86/amd64/ppc32/ppc64, and possibly MIPS, but I can't do the s390 bits. If I make available a patch containing the above stuff, can you do the s390 bits? Unfortunately this is not something that can be committed incrementally without breaking various front/back ends. > I'm willing to make the change if you and/or other port maintainers are > willing to double-check. Yes, I'm willing to double-check/review. J |
|
From: Florian K. <br...@ac...> - 2013-01-06 16:04:09
|
On 01/06/2013 10:48 AM, Julian Seward wrote:
>
>>> One change I am looking at is changing the guard type of Mux0X
>>> from I8 to I1. This makes it consistent with I1 guard types for
>>> Dirty helpers and for Exits.
>>
>> Seconded.
>
> I have this working now for ARM, and, obviously, the generic IR
> stuff has been fixed up too. I can do the front/back end fixes
> for x86/amd64/ppc32/ppc64, and possibly MIPS, but I can't do the
> s390 bits. If I make available a patch containing the above stuff,
> can you do the s390 bits?
Sure, no problem.
>> I'm willing to make the change if you and/or other port maintainers are
>> willing to double-check.
>
> Yes, I'm willing to double-check/review.
OK, let's do it them. Do you want to merge COMEM first ?
How about this (with ITE meaning if-then-else)?
Index: VEX/pub/libvex_ir.h
===================================================================
--- VEX/pub/libvex_ir.h (revision 2627)
+++ VEX/pub/libvex_ir.h (working copy)
@@ -1578,7 +1578,7 @@
Iex_Unop,
Iex_Load,
Iex_Const,
- Iex_Mux0X,
+ Iex_ITE,
Iex_CCall
}
IRExprTag;
@@ -1762,18 +1762,18 @@
IRExpr** args; /* Vector of argument expressions. */
} CCall;
- /* A ternary if-then-else operator. It returns expr0 if cond is
- zero, exprX otherwise. Note that it is STRICT, ie. both
- expr0 and exprX are evaluated in all cases.
+ /* A ternary if-then-else operator. It returns iftrue if cond is
+ zero, iffalse otherwise. Note that it is STRICT, ie. both
+ iftrue and iffalse are evaluated in all cases.
- ppIRExpr output: Mux0X(<cond>,<expr0>,<exprX>),
- eg. Mux0X(t6,t7,t8)
+ ppIRExpr output: ITE(<cond>,<iftrue>,<iffalse>),
+ eg. ITE(t6,t7,t8)
*/
struct {
IRExpr* cond; /* Condition */
- IRExpr* expr0; /* True expression */
- IRExpr* exprX; /* False expression */
- } Mux0X;
+ IRExpr* iftrue; /* True expression */
+ IRExpr* iffalse; /* False expression */
+ } ITE;
} Iex;
};
@@ -1808,7 +1808,7 @@
extern IRExpr* IRExpr_Load ( IREndness end, IRType ty, IRExpr* addr );
extern IRExpr* IRExpr_Const ( IRConst* con );
extern IRExpr* IRExpr_CCall ( IRCallee* cee, IRType retty, IRExpr**
args );
-extern IRExpr* IRExpr_Mux0X ( IRExpr* cond, IRExpr* expr0, IRExpr*
exprX );
+extern IRExpr* IRExpr_ITE ( IRExpr* cond, IRExpr* expr0, IRExpr*
exprX );
/* Deep-copy an IRExpr. */
extern IRExpr* deepCopyIRExpr ( IRExpr* );
|
|
From: Julian S. <js...@ac...> - 2013-01-07 08:04:10
|
On Sunday, January 06, 2013, Florian Krohm wrote: > >> I'm willing to make the change if you and/or other port maintainers are > >> willing to double-check. > > > > Yes, I'm willing to double-check/review. > > OK, let's do it them. Do you want to merge COMEM first ? Yes. (Unfortunately) I think it would be wise to merge COMEM first, and also to finish off and commit the guard-type change too. Without those in place, there is going to be a lot of conflicts with a name-and-arg-order change. I hope to get them done this week. > How about this (with ITE meaning if-then-else)? ITE as a name seems fine. w.r.t. the comment .. > + /* A ternary if-then-else operator. It returns iftrue if cond is > + zero, iffalse otherwise. s/zero/nonzero, I think. J |
|
From: Florian K. <br...@ac...> - 2013-01-07 20:18:58
|
On 01/07/2013 03:03 AM, Julian Seward wrote: > On Sunday, January 06, 2013, Florian Krohm wrote: > >> OK, let's do it them. Do you want to merge COMEM first ? > > Yes. (Unfortunately) I think it would be wise to merge COMEM > first, and also to finish off and commit the guard-type change > too. Without those in place, there is going to be a lot of > conflicts with a name-and-arg-order change. I hope to get > them done this week. No rush here. I have a largish DFP patch waiting for me in BZ ... > ITE as a name seems fine. w.r.t. the comment .. > >> + /* A ternary if-then-else operator. It returns iftrue if cond is >> + zero, iffalse otherwise. > > s/zero/nonzero, I think. Righto. Florian |