[PATCH] D70947: Add a default address space for globals to DataLayout

Wed Aug 26 09:26:50 PDT 2020

dylanmckay added a comment.

> I've split the backwards compat tests and AMDGPU changes out to D84345 <https://reviews.llvm.org/D84345>. Is the DataLayout upgrade what you meant with bitcode compatibility tests or should I be adding additional tests?

Good point - comparing this to D84545 <https://reviews.llvm.org/D84545>, there seems to be no obvious reason to add bitcode compatibility tests in this patch because the bitcode format itself is not changed (the data layout is just an arbitrary-length string embedded within the bitcode file). This particular patch does not need bitcode compatibility tests.

As far as I'm concerned, the code in this patch is good to go and all of the comments have been resolved, although I will defer actually approving the patch to be merged until the comments on D84345 <https://reviews.llvm.org/D84345> are sorted, which I think is important so that the new logic is guaranteed to have at least one in-tree use. Once D84345 <https://reviews.llvm.org/D84345> has been approved, I will formally hit approve on this patch and they can both be merged at the same time in a set.

================
Comment at: llvm/lib/IR/Globals.cpp:369
+                       ? *AddressSpace
+                       : M.getDataLayout().getDefaultGlobalsAddressSpace()),
       isConstantGlobal(constant),
----------------
dylanmckay wrote:
> Can we add a test that global variables do indeed get initialized with the address space specified in the DataLayout?
> 
> For example, compile under X86 but with `G7` in the data layout, then assert `addrspace(7)` in the IR output.
> 
> I think every other code path is covered.
> Can we add a test that global variables do indeed get initialized with the address space specified in the DataLayout?
> 
> For example, compile under X86 but with `G7` in the data layout, then assert `addrspace(7)` in the IR output.
> 
> I think every other code path is covered.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70947/new/

https://reviews.llvm.org/D70947