This is an archive of the discontinued LLVM Phabricator instance.

Implement Named Register Global Variables in LLVM
ClosedPublic

Authored by rengolin on Apr 2 2014, 7:06 AM.

Details

Summary

Hi all,

This is the first attempt at implementing named register globals in LLVM. There are a number of hacks that will need to be pruned and properly implemented and a number of questions that I have in the code about how to better achieve this. They're commented inline as FIXME.

The key design decisions:

  1. The LangRef change explains the semantics of the change. Basically:
    1. The register name is a private global string
    2. The value on write is a pointer type, to get the correct size from the target description
    3. The front end is responsible for casting it (ptrtoint/inttoptr) when needed
  2. I'm using an intrinsic for read/write from registers, so that way there will be no side effects from emitting them, nor will it allow taking the address of the register variable.
  3. I'm using a global string as the description, as it was the simplest IR I could come up with. I couldn't find a way to have literal strings ([3 x i8]* "sp\0") or how to embed metadata as a node. The problem with this is that the constant is still being emitted in the final file.
    1. If the constant is fine, I need to remove it from the DAG at the right moment. On getRegisterStringValue() is a bit too soon, as it breaks the DAG shortly after. I'll check when it's safe to call the clean up routines.
    2. If metadata is better, to avoid dangling data sections, how do I embed it in the intrinsic call?
  4. SelectionDAG -> Lower Intrinsic will lower the intrinsic call into either READ_REGISTER or WRITE_REGISTER nodes. The former is a simple replacement, the latter needs to become a DAG root and long the chain from the value's chain.
    1. I'm not sure that's the best way to catch the chain from the value, which could be anything, not just a copy from virtual register.
  5. TargetLowering simply converts READ -> CopyFromReg and WRITE -> CopyToReg(Value)
    1. getRegisterStringValue encodes the knowledge of reading the literal string from the global/metadata
    2. getRegisterByName encodes the logic of which registers are valid. This could be a TableGen property and auto-generated
  6. On all targets, lowering READ/WRITE nodes and getting the string literal are identical. Is there a place I can common them up?

Welcoming any comment. Thanks!

Diff Detail

Event Timeline

rengolin updated this revision to Unknown Object (????).Apr 4 2014, 5:11 AM

New in this revision:

  • Changing it to use metadata instead of global string, avoids emitting the string data at the end.
  • Special case for a register named "stack" to map to the architecture stack pointer and to be used with __builtin_stack_pointer.

I'd still prefer to not assert on getRegisterName()

Should be considered more serious for commit now.

rengolin updated this revision to Unknown Object (????).Apr 4 2014, 6:48 AM

llvm_unreachable is slightly better...

Hi Renato,

I like the metadata change.

On all targets, lowering READ/WRITE nodes and getting the string literal are identical. Is there a place I can common them up?

I'd be tempted to make them legal and handle it in SelectionDAGISel. There's a function SelectCodeCommon that seems to deal with the (similar INLINEASM) nodes.

On the llvm_unreachable thing, how about using "report_fatal_error" instead? Might be a bit friendlier if the front-end isn't doing the validation.

Also, could we have ARM64 support in there? That's most likely going to be the long-term AArch64 backend, and it'd be good not to have to merge it across manually.

Cheers.

Tim

docs/LangRef.rst
6811–6812

I'd make it polymorphic. I suspect quite a few targets have pointer types that are inadequate to represent all registers (for various reasons).

6818–6820

Undefined is undefined. There's no wiggle-room to it. "Unspecified" is probably the closest C standard term here.

lib/Target/AArch64/AArch64ISelLowering.cpp
2426

I don't think "xsp" exists (even in GNU as). The 64-bit stack pointer is universally referred to as "sp" as far as I can tell.

rengolin added inline comments.Apr 25 2014, 9:57 AM
docs/LangRef.rst
6811–6812

Wouldn't that force me to write many definitions of the intrinsics in Intrinsics.td?

I'm assuming the pointer is the size of the generic purpose registers, which we'll support with this. If we want to support other registers (vector, etc) we should create other intrinsics.

rengolin updated this revision to Diff 8851.Apr 25 2014, 10:02 AM
rengolin edited edge metadata.

Re-writing the patch after Tim's comments. Changes are:

  • DAG node is now legal and dealt with by SelectionDAGISel
  • Each TargetLowering restricts the register names locally
  • Generic TargetLowering restricts all (default implementation)
  • Using report_fatal_error instead of llvm_unreachable
  • ARM64 support and test
  • Changed wording in LangRef
  • Fixed some typos

The only change not done is the type (from pointer to polymorphic), discussions should continue.

Works well, but generates an invalid warning:

register unsigned long sp asm ("sp");

warning: variable 'sp' is uninitialized when used here [-Wuninitialized]

Is there a way this can be fixed without turning off this check?

Behan

Hi Behan,

This patch should not work yet. It's just the low-level implementation for the feature. We still need the Clang changes to create the metadata/intrinsic calls from the named register usage, and that's another step. You should still use the inline asm hack for now.

cheers,
--renato

to and will abort in case the register is not supported or not existent

We always reject at compile time.

Pointer types are used to make sure it has the same bit width as the

register itself.

Having i32 and i64 versions seems better.

Warning: There is no register reservation at the moment, so it only

works reliably on non-allocatable registers.

We just just error for now if the register is allocatable.

What was the use case for writing to registers? Maybe split that to
another patch?

Do we need the "stack" pseudo register? If it is only to implement
__builting_stack_pointer, it seems better to do it there. In fact, it
could be implemented as regular function once this is in, no?

Hi Rafael,

I'll try to make two versions (32/64) and see how it goes.

About the "stack" pseudo register, I created it because I didn't want to burden the front-end to detect what class of registers are valid on each backend, and since I was already checking it, why not? Even if Clang has this knowledge, not necessarily all other front-ends might want to, and they might want to use the stack register in target independent code.

cheers,
--renato

rengolin updated this revision to Diff 9032.May 2 2014, 6:25 AM
rengolin edited edge metadata.

Making type polymorphic (as Tim and Rafael suggested), plus amending the docs.

Can you explicitly check for allocatable registers? That should let us include a test showing that eax (for example) is rejected with a message saying it is an allocatable register. That would be a nice safety if future developments use tablegen to find the register.

I am still not convinced of the value of the "stack" pseudo register. All current use cases we want this for already use an explicit register name when being compiled with gcc, no? Can we at least start without it? These things are far easier to add than to remove.

docs/LangRef.rst
6836

Out of date comment.

6843

Not sure what exactly these two paragraphs mean. It would probably be best to just say that

  • Allocatable registers are rejected.
  • The compiler does no extra checking than what would be available in an inline assembly statement.
6887

Just say that the same restrictions form llvm.read_register apply so they don't get out of sync.

rengolin updated this revision to Diff 9061.May 4 2014, 2:29 AM

Changes:

  • Merging docs to make it easier to not get them outdated
  • Removed the "stack" register name for now

Not changed:

  • It still doesn't check for allocatable vs. non-allocatable, but it has a list of registers it supports and emit an error if the required register is not on the list. This is not the best solution but it's a placeholder for a better implementation, which should be on another patch.

For better error reporting, the front-end could do some checking. The errors from the back-end are quite ugly and I'd rather it not be the first line of defence in any case.

rengolin removed a subscriber: rafael.
rafael edited edge metadata.May 5 2014, 10:03 AM

+Warning: There is no register reservation at the moment, so it only

+works reliably on non-allocatable registers.

This is not entirely correct. It is not that allocatable registers
"don't work reliably". We reliably error on them. This should probably
be something like "the use of allocatable registers is rejected".

Other than that all the is missing I thing are tests for the errors.
Please include at lest two. One with a truly wrong name and one with
an allocatable register. On the test with the allocatable register
include a FIXME about producing a more specific error message.

Good point. I'll add some negative tests.

About the non-allocatable registers, to be fair, the *only* register that is accepted right now is the stack pointer. Should I add to each architecture the list of non-allocatable ones, too? Or should I just update the docs saying that, for now, only the stack pointer works?

rengolin updated this revision to Diff 9104.May 6 2014, 2:44 AM
rengolin edited edge metadata.

+ Adding some negative tests
+ Changing docs to reflect on stack register usage only

The latter was preferred to adding more non-allocatable registers, since there aren't many that could be added without further consideration and I don't want to delay this patch longer than it needs to. So, for the time being, only stack registers will be allowed as a named register.

Warning: There is no register reservation at the moment, and so far it
only works with the stack pointer on selected architectures (ARM, ARM64,
x86_64 and AArch64). Work to include a wider bank of registers is in
progress.

Please make it clear that work to include allocatable registers is
*not* under way. That is a much harder and brittler feature. We need
a full discussion an llvmdev before claiming that we may ever support
it.

on the named-reg-alloc.ll tests, please include a fixme about
producing a more specific error message about allocatable registers.

rengolin updated this revision to Diff 9108.May 6 2014, 5:53 AM

+ Remove future plans to avoid over-promising
+ Include FIXME for better allocatable-aware error messages (not yet because we only support one reg anyway)

+Warning: There is no register reservation at the moment, and so far it
+only works with the stack pointer on selected architectures (ARM, ARM64,
+x86_64 and AArch64).

This is still inaccurate. There are two restrictions that we should mention:

  • There is no reservation and we error on allocatable registers. That

is not "for now" or "at the moment".

  • The only thing that is currently implemented is the stack registers

in some architectures, but we hope to expand that to other
non-allocatable registers in the future.

rengolin updated this revision to Diff 9109.May 6 2014, 7:06 AM

+ Reduce even further the scope of the warning.

I don't want to add anything regarding future implementations to avoid people asking for it later.

rengolin updated this revision to Diff 9114.May 6 2014, 9:31 AM

Final? update on the docs to make sure allocatable registers are *not* supported.

rengolin accepted this revision.May 6 2014, 10:00 AM
rengolin added a reviewer: rengolin.

Committed r208104.

This revision is now accepted and ready to land.May 6 2014, 10:00 AM
rengolin closed this revision.May 6 2014, 10:00 AM