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

[Ben Shi via Phabricator via llvm-commits]

benshi001 added inline comments.


================
Comment at: llvm/lib/Target/AVR/AVRTargetMachine.h:57
+    // pointers.
+    return getPointerSize(SrcAs) == getPointerSize(DestAs);
+  }
----------------
aykevl wrote:
> 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).
I see. So only checking pointer size is good enough. Thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142107