[PATCH] D142107: [AVR] Support address space casts

[Ayke via Phabricator via llvm-commits]

aykevl added a comment.

In D142107#4067538 <https://reviews.llvm.org/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.



================
Comment at: llvm/lib/Target/AVR/AVRTargetMachine.h:57
+    // pointers.
+    return getPointerSize(SrcAs) == getPointerSize(DestAs);
+  }
----------------
benshi001 wrote:
> aykevl wrote:
> > benshi001 wrote:
> > > it would be better to also check the range, something like
> > > 
> > > ```
> > >    return SrcAs < AVR::NumAddrSpaces && DestAs < AVR::NumAddrSpaces &&
> > >           getPointerSize(SrcAs) == getPointerSize(DestAs);
> > > ```
> > 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?
> 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` ?
> 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).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142107