Page MenuHomePhabricator

Global Named Register Variables Support
ClosedPublic

Authored by rengolin on May 16 2014, 5:55 AM.

Details

Reviewers
rafael
rnk
Summary

Non-allocatable Global Named Register

This patch implements global named registers in Clang, lowering to the just
created intrinsics in LLVM (@llvm.read/write_register). A new type of LValue
had to be created (Register), which just adds support to carry the metadata
node containing the name of the register. Two new methods to emit loads and
stores interoperate with another to emit the named metadata node.

No guarantees are being made and only non-allocatable global variable named
registers are being supported. Local named register support is unchanged.

Diff Detail

Event Timeline

rengolin updated this revision to Diff 9472.May 16 2014, 5:55 AM
rengolin retitled this revision from to Global Named Register Variables Support.
rengolin updated this object.
rengolin edited the test plan for this revision. (Show Details)
rengolin added reviewers: rnk, rafael.
rengolin set the repository for this revision to rL LLVM.
rengolin added subscribers: behanw, mcharleb.
rengolin updated this revision to Diff 9487.May 16 2014, 8:58 AM

Simpler tests, better comments, names.

rnk added inline comments.May 16 2014, 1:09 PM
include/clang/AST/Decl.h
830

This fixes "error: reference to local variable 'current_stack_pointer' declared in enclosing context", right?

I think there's more to sort out here. The Doxygen comments here clearly say that this function only works on variables with function scope, but clearly somebody is calling it on variables without checking if it has function scope. We also have a check for isFileVarDecl() above, further supporting this. Probably the right fix is to return false early if (!isLocalVarDecl()).

lib/CodeGen/CGExpr.cpp
1351

Does Doxygen pick these up from source files? I thought it only worked in headers.

Anyway, we're not repeating the name of the function anymore. Instead we use \brief or @brief.

Is "Global Named Register" a proper noun deserving of capitalization?

1369

This condition will essentially never be true because there aren't many non-allocatable registers worth storing to. The only one I can think of is the TLS pointer on some ISAs, and this will only happen in libc or low-level libs. Let's move this into the !Dst.isSimple() check and put it *after* Dst.isBitField().

1603

The comment is copy-pasted and incorrect.

The same Doxygen advice applies.

1783

star-on-the-right here and for M.

rengolin added inline comments.May 18 2014, 4:44 AM
include/clang/AST/Decl.h
830

The function hasGlobalStorage() returns !hasLocalStorage(), and that's how we get here, so not only local scope here, I guess.

Also, I don't know if isLocalVarDecl will catch all "local" context, since there could be other local declarations.

While early return for !isLocalVarDecl() would be correct, !isFileVarDecl() is used only when the storage class is None, which makes me think that it's not as ubiquitous as it might transpire, and why I didn't add !isLocalVarDecl() on a global scope on that function, but inside a very specific case, which I know to be true.

If both have complete semantics, why not add on the first line:

if (!isLocalVarDecl() && !isFileVarDecl())
  return false;
lib/CodeGen/CGExpr.cpp
1351

I was just following the pattern around this function. I'll use \@brief.

1369

I put it as the last line, but it didn't follow that path, probably because of !isSimple(). I'll move it inside, as the last check.

1603

ok

1783

ok. I'll also move the initialization of Str inside the if.

rengolin updated this revision to Diff 9512.May 18 2014, 4:57 AM

Response to reviews:

  • Changed Doxygen comments
  • Pointer declaration star-on-right
  • Moved Str variable inside conditional that uses it
  • Moved isGlobalReg() inside !isSimple() just before assert(isBitfield)

Not changed yet (undergoing discussion):

  • if (getStorageClass() == SC_Register && !isLocalVarDecl())
rengolin updated this revision to Diff 9513.May 18 2014, 4:59 AM

Doxygen "brief" is either slash or at, not both.

rnk accepted this revision.May 19 2014, 10:55 AM
rnk edited edge metadata.

LGTM

include/clang/AST/Decl.h
830

OK, let's leave it alone.

This revision is now accepted and ready to land.May 19 2014, 10:55 AM
rengolin closed this revision.May 19 2014, 11:23 AM

Thanks Reid, r209149.