CodeGen X86: fix unsafe foreign calls wrt inlining
ClosedPublic

Authored by hsyl20 on May 24 2016, 8:24 PM.

Details

Summary

Foreign calls (unsafe and safe) interact badly with inlining and register passing ABIs (see Trac #11792 and Trac #12614):
the inlined code to compute a parameter of the call may overwrite a register already set to pass a preceding parameter.

With this patch, we compute all parameters which are not simple expressions before assigning them to fixed registers required by the ABI.

Test Plan
  • Add test (test both reg and stack parameters)
  • Validate

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.
hsyl20 updated this revision to Diff 7728.May 24 2016, 8:24 PM
hsyl20 retitled this revision from to CodeGen X86: fix unsafe foreign calls wrt inlining.
hsyl20 updated this object.
hsyl20 edited the test plan for this revision. (Show Details)
hsyl20 added reviewers: osa1, bgamari, simonmar.
hsyl20 updated the Trac tickets for this revision.
osa1 edited edge metadata.May 25 2016, 2:35 AM

I don't think this is a good solution. As far as I understand, this just pushes code that moves values of arguments to ABI arg registers down, so that they won't accidentally get overwritten by the code between the mov and callq.

Instead I think we should figure out why they're being overwritten in the first place. Like I mentioned in the trac ticket, I think callq instruction should have the information that edi, esi, edx and ecx is "read" by the instruction, so the register allocator should preserve those between the movs and call (by first allocating different register for temporaries, if that doesn't work, generating spill instructions etc.).

Let's discuss this a little bit more...

simonmar requested changes to this revision.May 25 2016, 3:24 AM
simonmar edited edge metadata.

Yes, so the problem is that the code generation for foreign calls is violating this rule:

https://phabricator.haskell.org/diffusion/GHC/browse/master/compiler/nativeGen/X86/CodeGen.hs;ac38c025b99868d348e3797e85a4af9c6d6377ac$2781-2782

You cannot assume that a fixed reg will stay live over an arbitrary computation.

Because an arbitrary computation can clobber certain fixed registers, which is exactly what divq does (it clobbers rax and rdx).

So what you're doing here fixes the problem, but it introduces extra reg-reg moves that the register allocator will not clean up, because it isn't that clever. I'd rather not make codegen worse for every foreign call, so we need to be a bit more clever here. I suggest:

  • Generate the code for the pushes first, so they can't clobber argument regs
  • Identify args that can't clobber: isOperand is the heuristic we usually use
  • Codegen any non-operand regs first into temporary regs. The last one can go directly into its arg reg.
  • Then assign the isOperand args, and the temporaries from the previous step, into the fixed argument regs

In the bad example, this should have the effect of generating the fourth arg first, followed by the others, and won't generate any extra instructions.

This revision now requires changes to proceed.May 25 2016, 3:24 AM
hsyl20 updated this revision to Diff 8797.Sep 27 2016, 9:23 AM
hsyl20 edited edge metadata.
  • Rebase
  • Fix to take into account Simon Marlow's suggestions
hsyl20 updated this revision to Diff 8798.Sep 27 2016, 9:25 AM
hsyl20 edited edge metadata.
  • Remove comment
hsyl20 updated this revision to Diff 8799.Sep 27 2016, 9:40 AM
hsyl20 edited edge metadata.
  • Fix too long lines
hsyl20 updated this object.Sep 27 2016, 10:01 AM
hsyl20 edited the test plan for this revision. (Show Details)
hsyl20 edited edge metadata.
hsyl20 updated the Trac tickets for this revision.
hsyl20 updated this revision to Diff 8800.Sep 27 2016, 10:13 AM
  • Add test
hsyl20 updated this revision to Diff 8801.Sep 27 2016, 10:22 AM
hsyl20 edited edge metadata.
  • Fix test
hsyl20 updated this revision to Diff 8802.Sep 27 2016, 11:07 AM
hsyl20 edited edge metadata.
  • Upload on staging
simonmar accepted this revision.Sep 28 2016, 4:26 AM
simonmar edited edge metadata.

This looks good. Thanks!

This revision is now accepted and ready to land.Sep 28 2016, 4:26 AM
This revision was automatically updated to reflect the committed changes.