This is an archive of the discontinued LLVM Phabricator instance.

Allow {e,r}bp as the target of {read,write}_register.
ClosedPublic

Authored by pgavlin on Jul 6 2015, 3:56 PM.

Details

Summary

This patch allows the read_register and write_register intrinsics to read/write the RBP/EBP registers on X86 iff the targeted register is the frame pointer for the containing function.

Diff Detail

Repository
rL LLVM

Event Timeline

pgavlin updated this revision to Diff 29135.Jul 6 2015, 3:56 PM
pgavlin retitled this revision from to Allow {e,r}bp as the target of {read,write}_register..
pgavlin updated this object.
pgavlin added reviewers: rengolin, nadav.
pgavlin set the repository for this revision to rL LLVM.
pgavlin added a subscriber: llvm-commits.
rnk added a subscriber: rnk.Jul 6 2015, 4:22 PM

Is it really that important to prevent people from hurting themselves by accessing ebp/rbp in a function without a frame pointer? I'd rather let them get hurt than suffer the extra code in LLVM.

test/CodeGen/X86/named-reg-alloc.ll
15

This CHECK is disabled with CHECKL?

In D10977#200036, @rnk wrote:

Is it really that important to prevent people from hurting themselves by accessing ebp/rbp in a function without a frame pointer? I'd rather let them get hurt than suffer the extra code in LLVM.

I went back and forth on that a bit. The documentation seems a bit at odds with the current implementation:

The compiler doesn’t check for register availability or use of the used register in surrounding code, including inline assembly. Because of that, allocatable registers are not supported.

The stance articulated here matches your suggestion (and indeed, my original change did not have the extra checks), but AFAICT the code in LLVM today (certainly for the X86 backend) is careful to reject allocatable registers. I erred on the side of "allocatable registers are not supported" rather than "the compiler doesn't check for reigster availability". That said, I really don't have a preference here--if you feel strongly that the additional checks are overkill, I'll remove them.

test/CodeGen/X86/named-reg-alloc.ll
15

Good catch.

rnk added a comment.Jul 6 2015, 5:35 PM

I think we can just allow rbp unconditionally.

pgavlin updated this revision to Diff 29145.Jul 6 2015, 7:39 PM
pgavlin removed rL LLVM as the repository for this revision.

Address CR feedback.

hfinkel added a subscriber: hfinkel.Jul 6 2015, 8:23 PM

You're going to need something in between this patch and the original one. The fundamental issue here is that asserts are not appropriate for user-facing code. If the user requests a read of 'ebp' in a function in which ebp is allocatable, then (at least in a +Asserts build) the verifier is likely to complain about a read of an undefined register. It is important that we prevent this from happening.

However, when this happens, we should try to give a slightly-more-informative error than, "Invalid register name global variable", because that's misleading. Please make it say something accurate for this case.

pgavlin updated this revision to Diff 29195.Jul 7 2015, 11:10 AM
pgavlin set the repository for this revision to rL LLVM.

Respond to Hal's feedback re: error conditions.

hfinkel added inline comments.Jul 7 2015, 11:17 AM
lib/Target/X86/X86ISelLowering.cpp
16230

This variable will be unused in a build without asserts, right? If so, you'll break those builds (many of them also build with -Werror). I think you might as well surround the entire body of this conditional with #ifndef NDEBUG.

pgavlin updated this revision to Diff 29205.Jul 7 2015, 12:55 PM

Moved frame register assertions into an NDEBUG guard.

hfinkel accepted this revision.Jul 8 2015, 6:50 PM
hfinkel added a reviewer: hfinkel.

LGTM.

This revision is now accepted and ready to land.Jul 8 2015, 6:50 PM
This revision was automatically updated to reflect the committed changes.