Page MenuHomePhabricator

[X86ISelLowering] Fix TLSADDR lowering when shrink-wrapping is enabled
ClosedPublic

Authored by davide on Feb 18 2016, 4:48 PM.

Details

Reviewers
qcolombet
Summary

This is one of the blockers for the release. Testcase will follow. I'm running 'check-llvm' now.

Diff Detail

Event Timeline

davide updated this revision to Diff 48429.Feb 18 2016, 4:48 PM
davide retitled this revision from to [X86ISelLowering] Fix TLSADDR lowering when shrink-wrapping is enabled.
davide updated this object.
davide added a reviewer: qcolombet.
davide added subscribers: llvm-commits, dim.
emaste added a subscriber: emaste.Feb 18 2016, 5:01 PM
qcolombet edited edge metadata.Feb 18 2016, 5:07 PM

Hi Davide,

Thanks for fixing this.
To give a bit of context here for other reviewers, the bug is that TLSADDR are lowered into calls in MC. As a result, we need to put the “call markers” around those instructions, otherwise shrink-wrapping may push the prologue and epilogue pass them.

This mostly looks good to me see my inline comment. It misses a test case of course, but I know you’re working on it.

Cheers,
-Quentin

lib/Target/X86/X86ISelLowering.cpp
22665

Add a comment saying that we want to replace TLSADDR into
adjust_stackdown
TLSADDR
adjust_stackup
and why. (You may use what I said to explain the problem.)

22675

"after the instruction", instead of “at the end of the basic block”.

davide updated this revision to Diff 48444.Feb 18 2016, 6:09 PM
davide edited edge metadata.
  • Addressed Quentin's comments.
  • Added a test.

This passes llvm-check.

davide updated this revision to Diff 48449.Feb 18 2016, 7:00 PM
  • Fix test.
qcolombet accepted this revision.Feb 19 2016, 9:42 AM
qcolombet edited edge metadata.

Hi Davide,

LGTM with a few nitpicks on the test case.

Thanks for fixing it!
Cheers,
-Quentin

lib/Target/X86/X86ISelLowering.cpp
22680

two spaces after CALLSEQ_END.

test/CodeGen/X86/tls-shrink-wrapping.ll
9

Add a comment on what this is testing.
In case the test breaks in the future, eg because of code gen changes, we want to know what was the intended behavior.

11

Please use opt -instnamer on the test case to have temporary variable names (easier to update than %[0-9]+).

11

Please get rid of the metadata (!…) unless they are required to reproduce the problem.

55

You should be able to remove all the !… stuff after removing the !… from the function.

This revision is now accepted and ready to land.Feb 19 2016, 9:42 AM
hans added a subscriber: hans.Feb 19 2016, 11:37 AM
davide closed this revision.Feb 19 2016, 4:50 PM