Operations like
int a, b, tmp;
tmp = a;
a = b;
b = tmp;
can be replaced by
int a, b;
swap(a, b);
This uses kernel.h macro definition and simplifies the code.
Thanks to Julia Lawall for suggestions about expressions and sgen
Signed-off-by: Fabian Frederick <[email protected]>
---
scripts/coccinelle/misc/swap.cocci | 62 ++++++++++++++++++++++++++++++++++++++
1 file changed, 62 insertions(+)
create mode 100644 scripts/coccinelle/misc/swap.cocci
diff --git a/scripts/coccinelle/misc/swap.cocci b/scripts/coccinelle/misc/swap.cocci
new file mode 100644
index 0000000..0f0929c
--- /dev/null
+++ b/scripts/coccinelle/misc/swap.cocci
@@ -0,0 +1,62 @@
+/// Use swap macro instead of complete operation to simplify the code
+///
+// # Generated with sgen
+//
+// Confidence: Moderate
+// Copyright: (C) 2015 Fabian Frederick. GPLv2.
+
+virtual patch
+virtual context
+virtual org
+virtual report
+
+@r depends on patch && !context && !org && !report@
+identifier i1, i2, tmp;
+type t1;
+@@
+
+- t1 tmp;
+<+... when any
+- tmp = i1;
+- i1 = i2;
+- i2 = tmp;
++ swap(i1, i2);
+...+>
+
+
+// ----------------------------------------------------------------------------
+
+@r_context depends on !patch && (context || org || report)@
+type t1;
+identifier i1, i2, tmp;
+position j0, j1;
+@@
+
+* t1 tmp@j0;
+<+... when any
+* tmp@j1 = i1;
+* i1 = i2;
+* i2 = tmp;
+...+>
+
+// ----------------------------------------------------------------------------
+
+@script:python r_org depends on org@
+j0 << r_context.j0;
+j1 << r_context.j1;
+@@
+
+msg = "swap.cocci"
+coccilib.org.print_todo(j0[0], msg)
+coccilib.org.print_link(j1[0], "")
+
+// ----------------------------------------------------------------------------
+
+@script:python r_report depends on report@
+j0 << r_context.j0;
+j1 << r_context.j1;
+@@
+
+msg = "WARNING: use swap() and remove temporary variable if it's not used elsewhere around line %s." % (j1[0].line)
+coccilib.report.print_report(j0[0], msg)
+
--
2.4.0
> On 31 May 2015 at 11:42 Julia Lawall <[email protected]> wrote:
>
>
> I propose the extended version below (not currently coccicheck friendly).
> the extra features are:
>
> 1. The original version requires i1 and i2 to be identifiers, eg x and y.
> This doesn't address the case where they are terms line x->a or x[b]. The
> fact that the original code contains assignments with both i1 and i2 on
> the left hand side is enough to ensure that they are appropriate arguments
> for swap. So they can be changed to expression metavariables.
>
> 2. The original patch rule always removed the tmp variable. This is not
> valid if the tmp variable is used for something else. The new semantic
> patch separates the introduction of swap (rule r) from the removal of the
> variable declaration (rule ok and the one folowing). The rule ok checks
> that this is a function containing an introduced call to swap, and then
> the rule after removes the declaration if the variable is not used for
> anything else. Note that the name of the tmp variable is remembered in
> the invalid three-argument version of sawp. This is then cleaned up in
> the rule below.
>
> 3. The original patch always removed the initialization of the tmp
> variable. Actually, some code uses the tmp variable afterwards to refer
> to the old value. In the new semantic patch, the first set of rules
> considers the cases where the tmp variable is not used, and the last rule
> is for the case where the tmp variable is stll needed. No cleaning up of
> the declaration is needed in that case.
>
> There is one regression as compared to the original semantic patch: In the
> file lib/mpi/mpi-pow.c, the temporary variable is not needed after the
> change, but it is also not removed. It is declared within a loop, and
> Coccinelle does not realize that it is not needed afterwards, because it
> is needed on subsequent loop iterations. Trying to adjust the semantic
> patch to address this issue made it much slower and didn't fix the
> problem. Perhaps it is easier to rely on gcc to give an unused variable
> warning, and to clean it up then.
>
> Fabian, if you are o with this, do you want to sgenify it ans submit a new
> patch?
Hello Julia,
Interesting improvements :) Do we really need tmp variable in swap() ?
I tried the version below which removes swap(x,y,tmp)->swap(x,y) rule
and had the same result on drivers branch plus it solved a missing tmp.
Maybe you want to avoid out of context swap() calls ?
Regards,
Fabian
@r@
expression i1, i2, E;
identifier tmp;
@@
- tmp = i1;
- i1 = i2;
- i2 = tmp;
+ swap(i1, i2);
... when != tmp
? tmp = E
@ok exists@
type t1;
identifier r.tmp;
expression i1,i2;
position p;
@@
t1@p tmp;
...
swap(i1, i2);
@@
expression i1,i2;
identifier tmp;
type t1;
position ok.p;
@@
-t1@p tmp;
<... when strict
when != tmp
swap(i1, i2);
...>
// tmp variable still needed
@@
expression i1, i2;
identifier tmp;
@@
tmp = i1;
- i1 = i2;
- i2 = tmp;
+ swap(i1, i2);
>
> thanks,
> julia
>
> // it may be possible to remove the tmp variable
>
> @r@
> expression i1, i2, E;
> identifier tmp;
> @@
>
> - tmp = i1;
> - i1 = i2;
> - i2 = tmp;
> + swap(i1, i2, tmp);
> ... when != tmp
> ? tmp = E
>
> @ok exists@
> type t1;
> identifier r.tmp;
> expression i1,i2;
> position p;
> @@
>
> t1@p tmp;
> ...
> swap(i1, i2, tmp);
>
> @@
> expression i1,i2;
> identifier tmp;
> type t1;
> position ok.p;
> @@
>
> -t1@p tmp;
> <... when strict
> when != tmp
> swap(i1, i2, tmp);
> ...>
>
> @depends on r@
> expression i1,i2;
> identifier tmp;
> @@
>
> swap(i1,i2
> - ,tmp
> )
>
> // tmp variable still needed
>
> @@
> expression i1, i2;
> identifier tmp;
> @@
>
> tmp = i1;
> - i1 = i2;
> - i2 = tmp;
> + swap(i1, i2);
On Fri, 5 Jun 2015, Fabian Frederick wrote:
>
>
> > On 31 May 2015 at 11:42 Julia Lawall <[email protected]> wrote:
> >
> >
> > I propose the extended version below (not currently coccicheck friendly).
> > the extra features are:
> >
> > 1. The original version requires i1 and i2 to be identifiers, eg x and y.
> > This doesn't address the case where they are terms line x->a or x[b]. The
> > fact that the original code contains assignments with both i1 and i2 on
> > the left hand side is enough to ensure that they are appropriate arguments
> > for swap. So they can be changed to expression metavariables.
> >
> > 2. The original patch rule always removed the tmp variable. This is not
> > valid if the tmp variable is used for something else. The new semantic
> > patch separates the introduction of swap (rule r) from the removal of the
> > variable declaration (rule ok and the one folowing). The rule ok checks
> > that this is a function containing an introduced call to swap, and then
> > the rule after removes the declaration if the variable is not used for
> > anything else. Note that the name of the tmp variable is remembered in
> > the invalid three-argument version of sawp. This is then cleaned up in
> > the rule below.
> >
> > 3. The original patch always removed the initialization of the tmp
> > variable. Actually, some code uses the tmp variable afterwards to refer
> > to the old value. In the new semantic patch, the first set of rules
> > considers the cases where the tmp variable is not used, and the last rule
> > is for the case where the tmp variable is stll needed. No cleaning up of
> > the declaration is needed in that case.
> >
> > There is one regression as compared to the original semantic patch: In the
> > file lib/mpi/mpi-pow.c, the temporary variable is not needed after the
> > change, but it is also not removed. It is declared within a loop, and
> > Coccinelle does not realize that it is not needed afterwards, because it
> > is needed on subsequent loop iterations. Trying to adjust the semantic
> > patch to address this issue made it much slower and didn't fix the
> > problem. Perhaps it is easier to rely on gcc to give an unused variable
> > warning, and to clean it up then.
> >
> > Fabian, if you are o with this, do you want to sgenify it ans submit a new
> > patch?
>
> Hello Julia,
>
> Interesting improvements :) Do we really need tmp variable in swap() ?
> I tried the version below which removes swap(x,y,tmp)->swap(x,y) rule
> and had the same result on drivers branch plus it solved a missing tmp.
> Maybe you want to avoid out of context swap() calls ?
>
> Regards,
> Fabian
>
> @r@
> expression i1, i2, E;
> identifier tmp;
> @@
>
> - tmp = i1;
> - i1 = i2;
> - i2 = tmp;
> + swap(i1, i2);
> ... when != tmp
> ? tmp = E
>
> @ok exists@
> type t1;
> identifier r.tmp;
> expression i1,i2;
> position p;
> @@
>
> t1@p tmp;
> ...
> swap(i1, i2);
There is no connection between tmp and swap here. Tmp does have the same
name as some variable used in a previous swap, but there is no guarantee
that this swap here is the one that was introduced by the previous rule.
Unfortunately, when you make a transformation, all information about
positions is lost. So there is no way to put a position variable on a
generated swap and ensure that the one matched here is exactly the same.
Maybe you could go a little closer to ensuring that property by inheriting
i1 and i2 as well, and not just tmp.
julia
>
> @@
> expression i1,i2;
> identifier tmp;
> type t1;
> position ok.p;
> @@
>
> -t1@p tmp;
> <... when strict
> when != tmp
> swap(i1, i2);
> ...>
>
>
> // tmp variable still needed
>
> @@
> expression i1, i2;
> identifier tmp;
> @@
>
> tmp = i1;
> - i1 = i2;
> - i2 = tmp;
> + swap(i1, i2);
>
>
> >
> > thanks,
> > julia
> >
> > // it may be possible to remove the tmp variable
> >
> > @r@
> > expression i1, i2, E;
> > identifier tmp;
> > @@
> >
> > - tmp = i1;
> > - i1 = i2;
> > - i2 = tmp;
> > + swap(i1, i2, tmp);
> > ... when != tmp
> > ? tmp = E
> >
> > @ok exists@
> > type t1;
> > identifier r.tmp;
> > expression i1,i2;
> > position p;
> > @@
> >
> > t1@p tmp;
> > ...
> > swap(i1, i2, tmp);
> >
> > @@
> > expression i1,i2;
> > identifier tmp;
> > type t1;
> > position ok.p;
> > @@
> >
> > -t1@p tmp;
> > <... when strict
> > when != tmp
> > swap(i1, i2, tmp);
> > ...>
> >
> > @depends on r@
> > expression i1,i2;
> > identifier tmp;
> > @@
> >
> > swap(i1,i2
> > - ,tmp
> > )
> >
> > // tmp variable still needed
> >
> > @@
> > expression i1, i2;
> > identifier tmp;
> > @@
> >
> > tmp = i1;
> > - i1 = i2;
> > - i2 = tmp;
> > + swap(i1, i2);
>