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

Fri Apr 10 10:49:01 PDT 2020

tra added a comment.

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.

> 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`.

> NVPTX backend only handles the `internal` version.

This is obviously fixable.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77777