[AMDGPU] Set the default globals address space to 1

[arichardson]

Summary

This will ensure that passes that add new global variables will create them
in address space 1 once the passes have been updated to no longer default
to the implicit address space zero.
This also changes AutoUpgrade.cpp to add -G1 to the DataLayout if it wasn't
already to present to ensure bitcode backwards compatibility.

Depends on D70947

[arsenm]

llvm/lib/IR/AutoUpgrade.cpp

4386

I would expect datalayout upgrades to work by parsing the old string, and checking the field values inside. I guess directly checking the string isn't a new problem here

[arichardson]

llvm/lib/IR/AutoUpgrade.cpp

4386

I agree that would be less error prone. I wonder if there are cases where the old string may fail to parse so you have to do the textual upgrade first. I'm happy to make this change.

@akhuang is there a reason you used string parsing in D67631? Any objections to changing the code to parse the datalayout and add missing attributes?

[akhuang]

llvm/lib/IR/AutoUpgrade.cpp

4386

I don't think so; parsing the datalayout sounds better to me too.

[arichardson]

llvm/lib/IR/AutoUpgrade.cpp

4386

I just looked into parsing the DataLayout instead. Unfortunately the resulting code is more complicated since there are no setters in DataLayout and no way to create a normalized representation.
There's also no way to differentiate between no -G passed and -G0 so something like e-p:64:64-G0 will be converted to e-p:64:64-G0-G1

[dylanmckay]

llvm/lib/IR/AutoUpgrade.cpp

4386

I suspect it would be possible to use the existing DataLayout(StringRef) constructor on the string, then call getDefaultGlobalsAddressSpace() on it, explicitly ignoring modifying the datalayout for the special case of an explicit -G0.

For example,

cpp
  DataLayout ParsedDL = DataLayout(DL);
  if (T.isAMDGPU() && !DL.contains("-G0") &&ParsedDL.getDefaultGlobalsAddressSpace() != 1) {
    return DL.empty() ? std::string("G1") : (DL + "-G1").str();
  }

As I understand it, this would cover the fact that we cannot distinguish between an explicit default globals space of zero, and a datalayout without a default globals space (also DL::getDefaultGlobalsAddressSpace() == 0) by explicitly excluding the special case -G0

[dylanmckay]

llvm/lib/IR/AutoUpgrade.cpp

4386

To be completely correct it should not assume that the global address space is not at the very start of the data layout as my initial snippet did. I've removed the - prefix from the contains check

DataLayout ParsedDL = DataLayout(DL);
if (T.isAMDGPU() && !DL.contains("G0") &&ParsedDL.getDefaultGlobalsAddressSpace() != 1) {
  return DL.empty() ? std::string("G1") : (DL + "-G1").str();
}

[arichardson]

llvm/lib/IR/AutoUpgrade.cpp

4386

We then end up overwriting explicitly specified G<N> flags. Maybe some tests would like to check that overriding the globals address space works? Also I believe this can result in an invalid datalayout: "...-G2" will be converted to "...-G2-G1".

Not sure what the correct approach is @arsenm ?

[arichardson]

• fix failing tests after datalayout change

[arichardson]

clang-format

[arichardson]

ping?

[arichardson]

ping @arsenm . I'd really like to land D70947 so that I can upstream further changes and that review is blocked on this one.