Gobal register variables marked local

Hi Rafael,

I'm going down the route of marking the variable declaration as "named
register" in SemaDecl (ActOnVariableDeclarator) and then adding the
intrinsic at the initialization time.

Since it has no explicit initializer, it ends up in the
TentativeDefinitions array, which gets initialized in
ActOnEndOfTranslationUnit. But that's not what I want, since this
"variable" doesn't exist, per se, but I still need to keep it around
for replacing all reads/writes by an intrinsic.

Also, while parsing statements that contain a reference to this
variable, the diagnostics on the use of the global variable detect an
"undeclared local variable":

named_reg_global.c:4:10: error: reference to local variable
'current_stack_pointer' declared in enclosing context
  return current_stack_pointer;
named_reg_global.c:1:24: note: 'current_stack_pointer' declared here
register unsigned long current_stack_pointer asm("sp");

This looks like I'm not setting the right flags beforehand, so that
ParseStatement recognizes it's not a local variable, or just that I
still haven't found the right place to do the substitution.

ParseStatementOrDeclarationAfterAttributes has a switch with
"identifier" in it, but that doesn't seem a specific enough place to
add this logic. Any pointers?

The changes I made to stop marking the declaration invalid and to mark
it as "named register" is attached. Not that this would be the final
patch, but it's helping me understand the code.

cheers,
--renato

named-regs.patch (3.62 KB)

Please don’t make VarDecl ~4 bytes bigger just to support a rarely used gnu extension.

I think it might be better to model the asm blob as an initializer, which would save space and fix your tentative definition problems.

Please don't make VarDecl ~4 bytes bigger just to support a rarely used gnu
extension.

Hi Reid,

As I said, that's just a marker to help me find the right decl while
debugging. I wouldn't leave it in the final patch.

I think it might be better to model the asm blob as an initializer, which
would save space and fix your tentative definition problems.

The problem is that this "variable" will never be properly initialized
because it only refers to access to a register. So, what I need to do
is to keep the VarDecl around, maybe even lying about it's
initialization status, and detect that the variable is a named
register (SC_Register + AsmLableAttr) and emit the intrinsic instead.

I'm looking for the common place where all the reads/writes to
variables get lowered, so that I only change in one/two place/s.

cheers,
--renato

> Please don't make VarDecl ~4 bytes bigger just to support a rarely used
gnu
> extension.

Hi Reid,

As I said, that's just a marker to help me find the right decl while
debugging. I wouldn't leave it in the final patch.

Sorry, reading is hard.

I think it might be better to model the asm blob as an initializer, which
> would save space and fix your tentative definition problems.

The problem is that this "variable" will never be properly initialized
because it only refers to access to a register. So, what I need to do
is to keep the VarDecl around, maybe even lying about it's
initialization status, and detect that the variable is a named
register (SC_Register + AsmLableAttr) and emit the intrinsic instead.

I'm looking for the common place where all the reads/writes to
variables get lowered, so that I only change in one/two place/s.

It's not just lib/CodeGen/CGExpr.cpp? That seems like the kind of place
where we'd lower a regular old DeclRefExpr referring to a local or global
variable.

It looks like Sema is confused with your variable because it has
SC_Register for its storage class, which makes hasGlobalStorage() return
false. This is the relevant code:

  /// hasLocalStorage - Returns true if a variable with function scope
  /// is a non-static local variable.
  bool hasLocalStorage() const {
    if (getStorageClass() == SC_None)
      // Second check is for C++11 [dcl.stc]p4.
      return !isFileVarDecl() && getTSCSpec() == TSCS_unspecified;

    // Return true for: Auto, Register.
    // Return false for: Extern, Static, PrivateExtern,
OpenCLWorkGroupLocal.

    return getStorageClass() >= SC_Auto;
  }
...
  /// \brief Returns true for all variables that do not have local storage.
  ///
  /// This includes all global variables as well as static variables
declared
  /// within a function.
  bool hasGlobalStorage() const { return !hasLocalStorage(); }

Maybe hasLocalStorage() should be fixed to work on non-function scope
variables, or assert that it is only ever called on function scope
variables.

I think the isLocal issue was preventing me from getting there with
the right context. Now that I changed it to return global on named
registers it seems to work.

I'll report back when I've gone further.

Thanks!
--renato

Hi Reid,

So, following up my example...

  register unsigned long current_stack_pointer asm("sp");
  unsigned long get_stack_pointer_addr() {
    return current_stack_pointer;
  }

In this case, EmitReturnStmt() will call CreateStore(EmitScalarExpr())
which we don't need at this stage, but it doesn't matter because this
will be removed later by the backend.

The IR I want to generate is:

  %sp = call i32 @llvm.read_register.i32(metadata !0)
  ret i32 %sp

Though, this would also work:

  %ret = alloca i32*
  %0 = call i32 @llvm.read_register.i32(metadata !0)
  store i32 %0, i32* %ret
  ret i32 %ret

which is what I believe Clang is trying to do.

What I did was to go all the way down to:

  LValue CodeGenFunction::EmitDeclRefLValue(const DeclRefExpr *E);

adding an "if named register, emit a call to the intrinsic". However,
the result of the call is an RValue, not an LValue. But the return
statement is asking for the RValue, so it passes through a cast
(CK_LValueToRValue) because it's expecting the variable to have an
address, but ours doesn't.

This is the ast:

`-FunctionDecl 0x7e9f50 <line:3:1, line:5:1> line:3:15
get_stack_pointer_addr 'unsigned long ()'
  `-CompoundStmt 0x7ea050 <col:40, line:5:1>
    `-ReturnStmt 0x7ea030 <line:4:3, col:10>
      `-ImplicitCastExpr 0x7ea018 <col:10> 'unsigned long' <LValueToRValue>
        `-DeclRefExpr 0x7e9ff0 <col:10> 'unsigned long' lvalue Var
0x7e9e50 'current_stack_pointer' 'unsigned long'

I want the Var current_stack_pointer NOT to be an lvalue from the
beginning, so that the return gets directly the value itself, not a
cast to RValue.

Also, would be good to mark it volatile, but
NewVD->getType().addVolatile() doesn't seem to have effect.

cheers,
--renato

What about making a new LValue type that doesn't have an address? At the
source level, current_stack_pointer is an lvalue that can be stored to. If
we ever want to add support for the 'register' class specifier as it was
originally designed, we will need a new 'Register' LValue type that can be
read from and stored to. We probably don't want to do that, but it feels
like the right model to me.

Hi Reid,

I think I got it. Creating a new type of LValue wasn't that much
invasive after all, though I don't know how folks feel about it.
Attached is a patch that makes it work (but breaks other things and
has more hacks than I'd consider healthy). Can you have a look and see
if that's (more or less) what you had in mind? If that's the right
angle, I'll fix the bugs and add some tests.

cheers,
--renato

named-regs.patch (12 KB)

Yeah, this looks like a promising direction.