Movatterモバイル変換

bedevere-bot added the awaiting core review label

markshannon assignedpablogsal

markshannon changed the title~~bpo-41323: Move 'peephole' optimizations directly on the CFG.~~bpo-41323: Perform 'peephole' optimizations directly on the CFG.

Regenerate frozen importlib.

c480401

remilapeyre reviewed

Python/compile.c Outdated

		PyObject *names = NULL;
		PyObject *varnames = NULL;
		PyObject *name = NULL;
		PyObject *freevars = NULL;
		PyObject *cellvars = NULL;
		PyObject *bytecode = NULL;
		//PyObject *bytecode = NULL;

Copy link

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Did you forget this?

Copy link

MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Yes, thanks.

Remove commented out code.

6b7019a

Copy link

Contributor

rhettinger commentedJul 17, 2020

Mostly, this looks pretty good. Consider removing thepeephole.c file altogether. It was originally part ofcompile.c and got separated out when it got too large. We can more thePyCode_Optimize() function back tocompile.c. Ideally, we could drop it from the public api as part of Victor's PEP to overhaul the C API.

One small loss is that the current code has macros that provide meaningful opcode groupings like UNCONDITIONAL_JUMP, CONDITIONAL_JUMP, ABSOLUTE_JUMP, and JUMPS_ON_TRUE. I always found those grouping helpful for reasoning about the code.

pablogsal reviewed

Python/peephole.c Outdated

		Py_XINCREF(code);
		PyMem_Free(blocks);
		PyMem_Free(codestr);
		Py_INCREF(code);

Copy link

Member

pablogsalJul 17, 2020•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

You can usePy_UNUSED for the ones that are notcode in the function declaration.

Copy link

MemberAuthor

markshannonJul 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

done

pablogsal added the 🔨 test-with-buildbotsTest PR w/ buildbots; report in status section label

Copy link

bedevere-bot commentedJul 17, 2020

🤖 New build scheduled with the buildbot fleet by@pablogsal for commit6b7019a 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

bedevere-bot removed the 🔨 test-with-buildbotsTest PR w/ buildbots; report in status section label

pablogsal reviewed

Python/compile.c Outdated

Comment on lines 5885 to 5886

		if (!names \|\| !varnames)
		goto error;

Copy link

Member

pablogsalJul 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Suggested change

	if (!names\|\| !varnames)
	gotoerror;
	if (!names\|\| !varnames) {
	gotoerror;
	}

Copy link

MemberAuthor

markshannonJul 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

done

pablogsal reviewed

Python/compile.c

		optimize_cfg(struct assembler a, PyObject consts)
		{
		for (int i = 0; i < a->a_nblocks; i++) {
		if (optimize_basic_block(a->a_reverse_postorder[i], consts)) {

Copy link

Member

pablogsalJul 17, 2020•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Does this converge on one single iteration? What happens if you have a sequence of many jumps all together between blocks?

Copy link

MemberAuthor

markshannonJul 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

No. Some jumps that can be eliminated will still remain.

This PR is aimed at closely mimicking the behavior of the existing peephole optimizer.
We can improve it later.

pablogsal reviewed

Copy link

Member

pablogsal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

This looks great and very compact! This is a very good work@markshannon ! I will do a more thorough review over the weekend but I have left some minor comments for now

pablogsal reviewed

Python/compile.c

		@@ -3645,6 +3646,11 @@ compiler_boolop(struct compiler *c, expr_ty e)
		for (i = 0; i < n; ++i) {
		VISIT(c, expr, (expr_ty)asdl_seq_GET(s, i));
		ADDOP_JABS(c, jumpi, end);
		basicblock *next = compiler_new_block(c);

Copy link

Member

pablogsalJul 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Huh? Why we need a new blocks here for connecting the jumps?

Copy link

MemberAuthor

markshannonJul 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

All instructions following a jump or branch must start a new basic block.

We now need the CFG generated by the code generation pass to be correct, as we perform jump elimination on it, rather than the CFG recreated in peephole.c.

Longer term, I think the front end should use labels and CFG creation should be a pass in between the code generation pass and the optimizer.

Two minor code tidy ups.

a2915d5

markshannon force-pushed themove-peephole-optimizations-to-cfg branch from7845d6f to8551d33Compare

July 20, 2020 09:33

Copy link

MemberAuthor

markshannon commentedJul 20, 2020•
edited
Loading

@rhettinger
I've removed peephole.c as you suggested.

TheUNCONDITIONAL_JUMP,CONDITIONAL_JUMP,ABSOLUTE_JUMP, andJUMPS_ON_TRUE don't seem that useful in an instruction based optimizer.
I initially created versions of them, but it turned out they were unused. So I deleted them.
For jump-to-jump elimination, it turns out that the exact opcode is more important that whether a jump is conditional and/or relative.

Remove peephole.c

7bc2522

markshannon force-pushed themove-peephole-optimizations-to-cfg branch from8551d33 to7bc2522Compare

July 20, 2020 09:49

markshannon requested a review froma team as acode owner

July 20, 2020 09:49

sweeneyde reviewed

Jul 20, 2020

Python/compile.c

		}
		}
		}
		}

Copy link

Member

sweeneydeJul 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Don't we need one of these?

Suggested change

	}
	}
	PyObject_Free(stack);

Copy link

MemberAuthor

markshannonJul 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Yes, thanks.

Fix leak in mark_reachable.

fda0bed

pablogsal added the 🔨 test-with-buildbotsTest PR w/ buildbots; report in status section label

Jul 20, 2020

Copy link

bedevere-bot commentedJul 20, 2020

🤖 New build scheduled with the buildbot fleet by@pablogsal for commitfda0bed 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

bedevere-bot removed the 🔨 test-with-buildbotsTest PR w/ buildbots; report in status section label

Jul 20, 2020

rhettinger approved these changes

bedevere-bot removed the awaiting core review label

bedevere-bot added the awaiting merge label

pablogsal approved these changes

Treat jumps to empty blocks as jumps to the following block.

17541b6

Copy link

MemberAuthor

markshannon commentedJul 21, 2020

One final tweak. Jumps to empty blocks that were immediately followed by jumps were not being eliminated. The final commit fixes that.

Regenerate importlib

1b57a42

pablogsal approved these changes