Page MenuHomePhabricator

[JumpThreading] Ignore free instructions
ClosedPublic

Authored by nikic on Sep 22 2021, 1:52 PM.

Details

Summary

This is basically D108837 but for jump threading. Free instructions should be ignored for the threading decision. JumpThreading already skips some free instructions (like pointer bitcasts), but does not skip various free intrinsics -- in fact, it currently gives them a fairly large cost of 2.

Diff Detail

Event Timeline

nikic created this revision.Sep 22 2021, 1:52 PM
nikic requested review of this revision.Sep 22 2021, 1:52 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 22 2021, 1:52 PM

If desired, I could reduce this patch to just the TCC_Free check though.

Yes, please.

nikic updated this revision to Diff 374373.Sep 22 2021, 2:52 PM
nikic retitled this revision from [JumpThreading] Use cost model for jump threading decision to [JumpThreading] Ignore free instructions.
nikic edited the summary of this revision. (Show Details)

Only check TCC_Free, otherwise keep previous cost model.

lebedev.ri accepted this revision.Sep 22 2021, 11:32 PM

This LG.

I can see why it may be good to migrate to proper TTI costs here,
but the main question that raises are:

  1. in -Oz, should we be checking codesize cost?
  2. Changing heuristics will obviously affect optimization power. What does this does to the budget? Should we increase it to retain most of the current optimization power?
This revision is now accepted and ready to land.Sep 22 2021, 11:32 PM
This revision was landed with ongoing or failed builds.Sep 23 2021, 9:28 AM
This revision was automatically updated to reflect the committed changes.
hans added a subscriber: hans.Sep 24 2021, 7:26 AM

This caused compiler crash when building Chromium. See https://bugs.chromium.org/p/chromium/issues/detail?id=1252762#c8 for a reproducer, and the top of that bug for the stack dump.

I've reverted in 4604695d7c20e72b551a1a5224f3de877cb41bd3 in the meantime.

nikic added a comment.Sep 24 2021, 9:34 AM

@hans Do I understand correctly that this change was mistakenly reverted and can be reapplied now that the DSE issue is fixed?

hans added a comment.Sep 24 2021, 9:43 AM

@hans Do I understand correctly that this change was mistakenly reverted and can be reapplied now that the DSE issue is fixed?

Yes, it looks like the DSE change was the real issue. Sorry for the noise. I'll re-apply this one unless you beat me to it.