X Tutup
The Wayback Machine - https://web.archive.org/web/20260127194233/https://github.com/python/cpython/pull/23267
Skip to content

Conversation

@markshannon
Copy link
Member

@markshannon markshannon commented Nov 13, 2020

This PR:

  • Makes sure that the front-end produces a well-formed CFG. Previously it did not, which made optimization rather precarious.
  • Be more aggressive and systematic in the back-end:
    • Eliminate all LOAD_CONST const; conditional jump pairs.
    • When fusing jumps to jumps, immediately reanalyze the jump to see if further fusion is possible.
    • Eliminate NOPs at the end of a BB when safe to do so.

https://bugs.python.org/issue42349

Copy link
Member

Choose a reason for hiding this comment

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

I think we can not use lists in a news entry.

Copy link
Member

@terryjreedy terryjreedy Nov 14, 2020

Choose a reason for hiding this comment

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

Newlines in news entries are 'soft' so that each is re-wrapped to line lengths of (on my screen) 5 1/2 to 9 inches. (Shorter screen space get a horizontal scrollbar, longer has the extra space ignored.) The *s are not needed here anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for pointing that out. Fixed.

Comment on lines 772 to 775
Copy link
Member

Choose a reason for hiding this comment

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

To run all funcs and report each failure...

Suggested change
opcodes = list(dis.get_instructions(func))
self.assertEqual(2, len(opcodes))
self.assertIn('LOAD_', opcodes[0].opname)
self.assertEqual('RETURN_VALUE', opcodes[1].opname)
with self.subTest(func=func):
opcodes = list(dis.get_instructions(func))
self.assertEqual(2, len(opcodes))
self.assertIn('LOAD_', opcodes[0].opname)
self.assertEqual('RETURN_VALUE', opcodes[1].opname)

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

Copy link
Member

Choose a reason for hiding this comment

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

As near as I could tell, the only different between the two strings is the 1 or 2 lines of Contents, so that the two could be replaced by one f string with

{"   0: None\n" if sys.flags.optimize else "
 "   0: 'Formatted details of methods, functions, or code.'\n   1: None\n"}

Copy link
Member Author

Choose a reason for hiding this comment

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

You can't put backslashes in f-strings, apparently 😞

Copy link
Member

@terryjreedy terryjreedy Nov 14, 2020

Choose a reason for hiding this comment

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

You could use .format(...) instead, like we used to have to do. Or precalculate contents = ... before the string. But replacing " with ''' and either deleting \n or replacing with a literal newline should work. Revised suggested addition based on local testing:

{'''   0: None'''
if sys.flags.optimize else 
'''   0: 'Formatted details of methods, functions, or code.'
   1: None'''}

The newline after the closing } is added to either interpolation, so only one internal newline is needed between ''' quotes. The two trailing 'ns were a mistake. Put if ... else on a separate line or not; unquoted newlines between { and } do not matter.

@bedevere-bot
Copy link

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

@markshannon markshannon added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 17, 2020
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @markshannon for commit 4c61adc 🤖

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

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 17, 2020
@markshannon markshannon merged commit 266b462 into python:master Nov 17, 2020
adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 13, 2021
…-23267)

Make sure that CFG from compiler front-end is correct. Be a bit more aggressive in the compiler back-end.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

X Tutup