[PATCH] D77777: [nvptx] Add `nvvm.texsurf.handle` internalizer.

Fri Apr 10 13:26:44 PDT 2020

hliao added a comment.

In D77777#1974672 <https://reviews.llvm.org/D77777#1974672>, @tra wrote:

> In D77777#1972720 <https://reviews.llvm.org/D77777#1972720>, @hliao wrote:
>
> > In D77777#1972349 <https://reviews.llvm.org/D77777#1972349>, @tra wrote:
> >
> > > The patch could use a more detailed description. Specifically, it does not describe the purpose of these changes.
> > >
> > > > Replace them with the internal version, i.e. nvvm.texsurf.handle.internal just before the instruction selector.
> > >
> > > It's not clear what is 'them'. 'nvvm.texsurf.handle' ?
> > >  If so, do we need 'internal' any more? Can we just rename internal and be done with it? Adding an extra pass just to replace one intrinsic with another seems to be unnecessary.
> > >
> > > I may be missing something here. Why do we have internal and non-internal intrinsics at all? Do we need both?
> >
> >
> > besides required by NVVM IR spec,
>
>
> NVVM IR spec is for nvidia's own compiler. It's based on LLVM, but it does not impose direct constraints on LLVM's design choices.

It would be an advantage and, sometimes, desirable to generate IR compatible to NVVM IR spec.

> 
> 
>> the metadata in that intrinsic is a trick to prevent it from being sunk into common code during optimization in LLVM IR.
> 
> This sounds like it may have been done that way in an attempt to work around a problem with intrinsics' constraints. We may want to check if there's a better way to do it now.
>  Right now both intrinsics are marked with `[IntrNoMem]` which may be the reason for compiler feeling free to move it around. We may need to give compiler correct information and then we may not need this just-in-time intrinsic replacement hack. I think it should be at least `IntrArgMemOnly` or, maybe  `IntrInaccessibleMemOrArgMemOnly`.

That may not exactly model the behavior as, for binding texture/surface support, in fact, it's true that there's no memory operation at all. Even with InstArgMemOnly or similar attributes, it still won't be preventable for optimizations to sink common code. Such trick is played in lots of intrinsics, such as `read.register` and etc.

> 
> 
>> NVPTX backend only handles the `internal` version.
> 
> This is obviously fixable.

SDAG so far cannot handle metadata GISel doesn't have support either. Getting that supported in TICG won't justify too much for target-specific intrinsics as metadata should not directly be used in code generation.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77777