[AVR] Support address space casts

[aykevl]

Summary

All address spaces on AVR can be freely cast between (they keep the same bit pattern). They just aren't dereferenceable when they're in a different address space as they really do point to a separate address space.

This is supported in avr-gcc: https://godbolt.org/z/9Gfvhnhv9
(It crashes in Clang but that's a separate bug).

[benshi001]

I guess your crash in clang relates to https://github.com/llvm/llvm-project/issues/56426

[benshi001]

LGTM. You can adopt my suggestion while committing.

llvm/lib/Target/AVR/AVRTargetMachine.h

57

it would be better to also check the range, something like

return SrcAs < AVR::NumAddrSpaces && DestAs < AVR::NumAddrSpaces &&
       getPointerSize(SrcAs) == getPointerSize(DestAs);

[aykevl]

llvm/lib/Target/AVR/AVRTargetMachine.h

57

Please take a look at other architecture. Most of them make this a no-op without constraints, like here on ARM:

/// Returns true if a cast between SrcAS and DestAS is a noop.
bool isNoopAddrSpaceCast(unsigned SrcAS, unsigned DestAS) const override {
  // Addrspacecasts are always noops.
  return true;
}

Why would AVR be different?

[arsenm]

Commit message is a bit misleading, I'd expect "support address space casts" would mean also handling the not-noop cases

[benshi001]

llvm/lib/Target/AVR/AVRTargetMachine.h

57

It is just a suggestion, since I find

bool X86TargetMachine::isNoopAddrSpaceCast(unsigned SrcAS,
                                           unsigned DestAS) const {
  assert(SrcAS != DestAS && "Expected different address spaces!");
  if (getPointerSize(SrcAS) != getPointerSize(DestAS))
    return false;
  return SrcAS < 256 && DestAS < 256;
}

Is there some possibility that AVR's addrspace will exceed AVR::NumAddrSpaces ?

[aykevl]

In D142107#4067538, @arsenm wrote:

Commit message is a bit misleading, I'd expect "support address space casts" would mean also handling the not-noop cases

As far as I know, all pointers are currently 2 bytes (16 bits) in size. So in that sense it's a bit of a moot point.
Do you have a suggestion for a better title? Something like [AVR] Don't crash on address space casts doesn't seem great either.

llvm/lib/Target/AVR/AVRTargetMachine.h

57

Is there some possibility that AVR's addrspace will exceed AVR::NumAddrSpaces ?

I am not sure, but considering that other architectures like ARM and RISC-V simply return true that seems like a good behavior. AFAIK frontends also use address spaces for special purposes, there is more than just the address spaces the hardware supports.

I looked in the LangRef but surprisingly there is no documentation at all what an address space really is (because it is many different things).

[benshi001]

llvm/lib/Target/AVR/AVRTargetMachine.h

57

I see. So only checking pointer size is good enough. Thanks.

[aykevl]

This is merged but wasn't closed automatically, probably because I changed the commit message.

llvm/lib/Target/AVR/AVRTargetMachine.h

public:
   }
 
   TargetPassConfig *createPassConfig(PassManagerBase &PM) override;
 
   MachineFunctionInfo *
   createMachineFunctionInfo(BumpPtrAllocator &Allocator, const Function &F,
                             const TargetSubtargetInfo *STI) const override;
 
+  bool isNoopAddrSpaceCast(unsigned SrcAs, unsigned DestAs) const override {
+    // While AVR has different address spaces, they are all represented by
+    // 16-bit pointers that can be freely casted between (of course, a pointer
+    // must be cast back to its original address space to be dereferenceable).
+    // To be safe, also check the pointer size in case we implement __memx
+    // pointers.
+    return getPointerSize(SrcAs) == getPointerSize(DestAs);
+  }
+
 private:
   std::unique_ptr<TargetLoweringObjectFile> TLOF;
   AVRSubtarget SubTarget;
 };
 
 } // end namespace llvm
 
 #endif // LLVM_AVR_TARGET_MACHINE_H

llvm/test/CodeGen/AVR/addrspacecast.ll

+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc < %s -mtriple=avr | FileCheck %s
+
+@global = external global i8
+@progmem = external addrspace(1) global i8
+
+declare void @func() addrspace(1)
+declare void @doCallback(ptr) addrspace(1)
+
+define ptr @casttodata(ptr addrspace(1) %funcptr) addrspace(1) {
+; CHECK-LABEL: casttodata:
+; CHECK:       ; %bb.0:
+; CHECK-NEXT:    ret
+  %result = addrspacecast ptr addrspace(1) %funcptr to ptr
+  ret ptr %result
+}
+
+define ptr addrspace(1) @casttofuncptr(ptr %data) addrspace(1) {
+; CHECK-LABEL: casttofuncptr:
+; CHECK:       ; %bb.0:
+; CHECK-NEXT:    ret
+  %result = addrspacecast ptr %data to ptr addrspace(1)
+  ret ptr addrspace(1) %result
+}
+
+define ptr addrspace(1) @castglobal() addrspace(1) {
+; CHECK-LABEL: castglobal:
+; CHECK:       ; %bb.0:
+; CHECK-NEXT:    ldi r24, lo8(global)
+; CHECK-NEXT:    ldi r25, hi8(global)
+; CHECK-NEXT:    ret
+  %result = addrspacecast ptr @global to ptr addrspace(1)
+  ret ptr addrspace(1) %result
+}
+
+define ptr @castprogmem() addrspace(1) {
+; CHECK-LABEL: castprogmem:
+; CHECK:       ; %bb.0:
+; CHECK-NEXT:    ldi r24, lo8(progmem)
+; CHECK-NEXT:    ldi r25, hi8(progmem)
+; CHECK-NEXT:    ret
+  %result = addrspacecast ptr addrspace(1) @progmem to ptr
+  ret ptr %result
+}
+
+define ptr @castfunc() addrspace(1) {
+; CHECK-LABEL: castfunc:
+; CHECK:       ; %bb.0:
+; CHECK-NEXT:    ldi r24, pm_lo8(func)
+; CHECK-NEXT:    ldi r25, pm_hi8(func)
+; CHECK-NEXT:    ret
+  %result = addrspacecast ptr addrspace(1) @func to ptr
+  ret ptr %result
+}
+
+define void @callCallback() addrspace(1) {
+; CHECK-LABEL: callCallback:
+; CHECK:       ; %bb.0:
+; CHECK-NEXT:    ldi r24, pm_lo8(func)
+; CHECK-NEXT:    ldi r25, pm_hi8(func)
+; CHECK-NEXT:    rcall doCallback
+; CHECK-NEXT:    ret
+  call addrspace(1) void @doCallback(ptr addrspacecast (ptr addrspace(1) @func to ptr))
+  ret void
+}