[PATCH] D48803: Place the BlockAddress type in the program address space

[Dylan McKay via Phabricator via llvm-commits]

dylanmckay added a comment.

Nice patch, this looks useful!

> Yes I'm not sure I can make a test for this with any of the existing targets. I'll see if I can get something with AVR since that sets program address space to 1.

Here's a test for you that does it.

There's another bug in LLParser that stops nonzero program address spaces from working; if the function referenced in a block address is not known in the first pass of the LLParser (for example, when the blockaddress exists earlier in the IR file than the function definition, the LLParser must insert a forward reference for the function. It does this by creating a new global variable, but it unconditionally left the global variable in the default address space of zero.

The diff I have included has a fix for this.

I've also amended the LangRef docs so that they would be accurate under the new patch.

  diff --git a/docs/LangRef.rst b/docs/LangRef.rst
  index 06e092fb9fc..deac223d1a1 100644
  --- a/docs/LangRef.rst
  +++ b/docs/LangRef.rst
  @@ -3275,7 +3275,16 @@ Addresses of Basic Blocks
   ``blockaddress(@function, %block)``
   
   The '``blockaddress``' constant computes the address of the specified
  -basic block in the specified function, and always has an ``i8*`` type.
  +basic block in the specified function.
  +
  +It always has an ``i8 addrspace(P)*`` type, where ``P`` is the program
  +memory address space specified in the data layout. For targets that place
  +code and data in the same address space (Von-Neumann architectures) a block
  +address will have the same address space as data pointers, usually
  +``addrspace(0)``. Block addresses on targets that have different data and
  +code address spaces (Harvard architectures) will always be in the program
  +memory address space specified in the target's data layout.
  +
   Taking the address of the entry block is illegal.
   
   This value only has defined behavior when used as an operand to the
  diff --git a/lib/AsmParser/LLParser.cpp b/lib/AsmParser/LLParser.cpp
  index 5fe1e125d48..6581436c20f 100644
  --- a/lib/AsmParser/LLParser.cpp
  +++ b/lib/AsmParser/LLParser.cpp
  @@ -3154,9 +3154,13 @@ bool LLParser::ParseValID(ValID &ID, PerFunctionState *PFS) {
                                                 std::map<ValID, GlobalValue *>()))
                 .first->second.insert(std::make_pair(std::move(Label), nullptr))
                 .first->second;
  -      if (!FwdRef)
  +      if (!FwdRef) {
           FwdRef = new GlobalVariable(*M, Type::getInt8Ty(Context), false,
  -                                    GlobalValue::InternalLinkage, nullptr, "");
  +                                  GlobalValue::InternalLinkage, nullptr, "",
  +                                  nullptr, GlobalValue::NotThreadLocal,
  +                                  M->getDataLayout().getProgramAddressSpace());
  +      }
  +
         ID.ConstantVal = FwdRef;
         ID.Kind = ValID::t_Constant;
         return false;
  diff --git a/test/CodeGen/AVR/block-address-is-in-progmem-space.ll b/test/CodeGen/AVR/block-address-is-in-progmem-space.ll
  new file mode 100644
  index 00000000000..8e6e3a71062
  --- /dev/null
  +++ b/test/CodeGen/AVR/block-address-is-in-progmem-space.ll
  @@ -0,0 +1,51 @@
  +; RUN: llc -mcpu=atmega328 < %s -march=avr | FileCheck %s
  +
  +; This test verifies that the pointer to a basic block
  +; should always be a pointer in address space 1.
  +;
  +; If this were not the case, then programs targeting
  +; AVR that attempted to read their own machine code
  +; via a pointer to a label would actually read from RAM
  +; using a pointer relative to the the start of program flash.
  +;
  +; This would cause a load of uninitialized memory, not even
  +; touching the program's machine code as otherwise desired.
  +
  +target datalayout = "e-P1-p:16:8-i8:8-i16:8-i32:8-i64:8-f32:8-f64:8-n8-a:8"
  +
  +; CHECK-LABEL: load_with_no_forward_reference
  +define i8 @load_with_no_forward_reference(i8 %a, i8 %b) {
  +second:
  +  ; CHECK:      ldi r30, .Ltmp0+2
  +  ; CHECK-NEXT: ldi r31, .Ltmp0+4
  +  ; CHECK: lpm r24, Z
  +  %bar = load i8, i8 addrspace(1)* blockaddress(@function_with_no_forward_reference, %second)
  +  ret i8 %bar
  +}
  +
  +; CHECK-LABEL: load_from_local_label
  +define i8 @load_from_local_label(i8 %a, i8 %b) {
  +entry:
  +  %result1 = add i8 %a, %b
  +
  +  br label %second
  +
  +; CHECK-LABEL: .Ltmp1:
  +second:
  +  ; CHECK:      ldi r30, .Ltmp1+2
  +  ; CHECK-NEXT: ldi r31, .Ltmp1+4
  +  ; CHECK-NEXT: lpm r24, Z
  +  %result2 = load i8, i8 addrspace(1)* blockaddress(@load_from_local_label, %second)
  +  ret i8 %result2
  +}
  +
  +; A function with no forward reference, right at the end
  +; of the file.
  +define i8 @function_with_no_forward_reference(i8 %a, i8 %b) {
  +entry:
  +  %result = add i8 %a, %b
  +  br label %second
  +second:
  +  ret i8 0
  +}
  +


Repository:
  rL LLVM

https://reviews.llvm.org/D48803