This is an archive of the discontinued LLVM Phabricator instance.

[X86] Fix bug in x86_intrcc with arg copy elision
ClosedPublic

Authored by rnk on Jan 18 2019, 4:12 PM.

Details

Summary

Use a custom calling convention handler for interrupts instead of fixing
up the locations in LowerMemArgument. This way, the offsets are correct
when constructed and we don't need to account for them in as many
places.

Depends on D56883

Replaces D56275

Diff Detail

Repository
rL LLVM

Event Timeline

rnk created this revision.Jan 18 2019, 4:12 PM
wxiao3 added a subscriber: wxiao3.Jan 20 2019, 11:27 PM
rnk updated this revision to Diff 183175.Jan 23 2019, 2:17 PM
  • rebase

I don't know enough about LLVM to review the code, but I just built a custom Rust compiler that uses this patch and can verify that this fixes the passed error code. The passed exception stack frame looks correct too.

Thanks a lot for fixing this @rnk!

rnk added a comment.Jan 28 2019, 1:12 PM

@craig.topper, any thoughts? I think we'll want to merge this to 8.0 since Rust wants it.

craig.topper added inline comments.Jan 28 2019, 1:26 PM
llvm/test/CodeGen/X86/x86-32-intrcc.ll
121 ↗(On Diff #183175)

Was that stray A at the end of the line supposed to be another sentence? its in the other test file too.

llvm/test/CodeGen/X86/x86-interrupt_cc.ll
32 ↗(On Diff #183175)

Is there way to prevent the script from regexing over the addresses?

rnk updated this revision to Diff 183978.Jan 28 2019, 4:12 PM
rnk marked 2 inline comments as done.
  • remove stray character
llvm/test/CodeGen/X86/x86-32-intrcc.ll
121 ↗(On Diff #183175)

I think it's a stray Vim command for "insert at end of line". :)

llvm/test/CodeGen/X86/x86-interrupt_cc.ll
32 ↗(On Diff #183175)

No, it always wild-cards stack offsets, but optionally not global / RIP offsets. Do you remember having any particular reason to move to auto-generation in rL324530? I think the test only really needed to look for a few instructions, movk, and a few encodings. It doesn't make sense to me to auto-generate the tests if we card about the encoding, since the wildcarding does nothing to make the test less sensitive to unrelated changes.

I think I autogenerated it because I added more run lines and then fixed a bug in the next commit. I'm just in the habit of using the script.

So have we been incorrectly positioning the registers in the AVX512 tests?

rnk added a comment.Jan 29 2019, 2:12 PM

I think I autogenerated it because I added more run lines and then fixed a bug in the next commit. I'm just in the habit of using the script.

I think auto-generating them is fine, but we should stop checking the instruction encodings since it defeats the wildcarding. If it's valuable to check the encodings, we should have a separate, non-generated test for that.

So have we been incorrectly positioning the registers in the AVX512 tests?

So, after looking at the offsets, it uncovered a bug. I made a mistake to assume that all fixed stack objects come before the return address. I think I can tweak my X86FrameLowering.cpp change to retain the local area offset adjustment for spilled fixed stack objects (ones that would normally be part of the newly allocated frame, not arguments). I need to look into it more though.

rnk updated this revision to Diff 184183.Jan 29 2019, 2:49 PM
  • fix spill bug

What's the status of this?

Herald added a project: Restricted Project. · View Herald TranscriptFeb 25 2019, 3:42 AM
rnk added a comment.Feb 25 2019, 10:58 AM

What's the status of this?

I'm waiting for review from @craig.topper, but I also forgot about it.

This revision is now accepted and ready to land.Feb 25 2019, 11:19 AM
rnk added a comment.Feb 25 2019, 5:30 PM

Huh, apparently I spoke too soon, now the tests don't pass. :( Anyway, I'll move it forward.

This revision was automatically updated to reflect the committed changes.
rnk added a comment.Feb 25 2019, 6:38 PM
In D56944#1409973, @rnk wrote:

Huh, apparently I spoke too soon, now the tests don't pass. :( Anyway, I'll move it forward.

I did -= instead of +=, is all.