Change jump targets in JMP_TBL from blocks to X86.JumpDest.
ClosedPublic

Authored by AndreasK on Apr 14 2018, 10:09 AM.

Details

Summary

Jump tables always point to blocks when we first generate them.
However there are rare situations where we can shortcut one of
these blocks to a static address during the asm shortcutting
pass.

While we already updated the data section accordingly this patch
also extends this to the references stored in JMP_TBL.

Test Plan

ci

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
AndreasK created this revision.Apr 14 2018, 10:09 AM
AndreasK requested review of this revision.Apr 14 2018, 10:17 AM

Updating the references in JMP_TBL was introduced in https://phabricator.haskell.org/D4595.

I had mistakenly assumed that jump tables never contain immediate values based on the data type.
But in rare circumstances they can be shortcut to immediate values during asm level shortcutting.

This happens with tables next to code disabled when building GHC as shown by someone on GHC hitting this case.

bgamari requested changes to this revision.Apr 19 2018, 10:46 AM

Looks reasonable although I'm a bit worried that this invariant isn't discoverable enough.

compiler/nativeGen/X86/CodeGen.hs
2867

Is this invariant firmly documented somewhere? Perhaps we should cite a Note?

This revision now requires changes to proceed.Apr 19 2018, 10:46 AM
AndreasK added inline comments.Apr 20 2018, 11:42 AM
compiler/nativeGen/X86/CodeGen.hs
2867

Is this invariant firmly documented somewhere?

Sadly only by the code itself as far as I'm aware.

AndreasK updated the Trac tickets for this revision.Apr 30 2018, 8:18 AM
AndreasK updated this revision to Diff 16229.Apr 30 2018, 8:43 AM
  • Remove comment with invariant, panic in case JMP_TBL contains non label destination during static table generation.
AndreasK updated this revision to Diff 16231.Apr 30 2018, 8:46 AM
  • Remove comment with invariant.
AndreasK marked 2 inline comments as done.Apr 30 2018, 8:50 AM

For now I simply removed the comments with the invariant.

bgamari accepted this revision.May 16 2018, 6:16 PM

Alright, let's merge this.

This revision is now accepted and ready to land.May 16 2018, 6:16 PM
This revision was automatically updated to reflect the committed changes.