Missing TargetPrefix for NVVM intrinsics

[Justin Bogner via llvm-commits]

Justin Holewinski <jholewinski at nvidia.com> writes:
> Yeah, this is probably better handled on llvm-commits. Swapping mailing 
> lists. Any particular reason these aren't up on Phabricator?

Phabricator doesn't have a good way to deal with multiple related
patches AFAICT - you have to make multiple reviews and split the
discussion awkwardly. I also generally find it clunky and harder to deal
with than email, but some people disagree with me on that.

> Overall, the changes look good. A few comments:
>
>
> --- a/lib/Target/NVPTX/NVPTXInstrInfo.cpp
> +++ b/lib/Target/NVPTX/NVPTXInstrInfo.cpp
> @@ -112,8 +112,6 @@ bool NVPTXInstrInfo::isStoreInstr(const MachineInstr 
> &MI,
>
>   bool NVPTXInstrInfo::CanTailMerge(const MachineInstr *MI) const {
>     unsigned addrspace = 0;
> -  if (MI->getOpcode() == NVPTX::INT_CUDA_SYNCTHREADS)
> -    return false;
>     if (isLoadInstr(*MI, addrspace))
>       if (addrspace == NVPTX::PTXLdStInstCode::SHARED)
>
> This should be replaced with the bar0 intrinsic. Looks like a bug that 
> bar0 is missing anyway.

Sure, I'll update that. As a side note, it seems like this doesn't have
any test coverage.

> --- a/include/clang/Basic/BuiltinsNVPTX.def
> +++ b/include/clang/Basic/BuiltinsNVPTX.def
> @@ -15,50 +15,21 @@
>   // The format of this database matches clang/Basic/Builtins.def.
>
>   // Builtins retained from previous PTX back-end
>
> I would just remove the comment about being retained from the previous 
> back-end. The intent is to remove all legacy compatibility.

Sure. I should point out that while most of these swap to the equivalent
__nvvm name, several builtins don't seem to have alternatives, namely
read_laneid and other read_*ids, lanemask_<comparison>s, read_clock,
read_pm{0-3}, and read_bar_sync. This is related to jlebar's point about
some functionality appearing to be missing.

> --- a/test/Analysis/DivergenceAnalysis/NVPTX/diverge.ll
> +++ b/test/Analysis/DivergenceAnalysis/NVPTX/diverge.ll
> @@ -93,20 +93,20 @@ merge:
>   ; int i = 0;
>   ; do {
>   ;   i++;                  // i here is uniform
> -; } while (i < laneid);
> +; } while (i < tid);
>   ; return i == 10 ? 0 : 1; // i here is divergent
>   ;
>   ; The i defined in the loop is used outside.
>   define i32 @loop() {
>   ; CHECK-LABEL: Printing analysis 'Divergence Analysis' for function 'loop'
>   entry:
> -  %laneid = call i32 @llvm.ptx.read.laneid()
> +  %tid = call i32 @llvm.nvvm.read.ptx.sreg.tid.x()
>     br label %loop
>   loop:
>     %i = phi i32 [ 0, %entry ], [ %i1, %loop ]
>   ; CHECK-NOT: DIVERGENT: %i =
>     %i1 = add i32 %i, 1
> -  %exit_cond = icmp sge i32 %i1, %laneid
> +  %exit_cond = icmp sge i32 %i1, %tid
>     br i1 %exit_cond, label %loop_exit, label %loop
>   loop_exit:
>     %cond = icmp eq i32 %i, 10
>
> What is the intent of the change from laneid to tid?

The test doesn't care what intrinsic's being used - it's just checking
that divergence does the right thing around this kind of control flow.
As I noted above, nvvm.read.ptx.sreg.laneid doesn't seem to exist.

>
>
> On 7/5/2016 5:56 PM, Justin Bogner wrote:
>> Justin Lebar <jlebar at google.com> writes:
>>>> @jlebar, any issues on your end with removing all of the non-nvvm intrinsics?
>>> I don't think so, if the functionality is all there from the nvvm
>>> intrinsics.  Some if it may involve clang changes, which I'm not
>>> exactly volunteering to do, but they should be simple.
>> Okay, so if I've understood you both correctly, the attached six patches
>> should do the trick, for both LLVM and clang. First, they rename the
>> shfl intrinsics from __builtin_ptx_shfl to __nvvm_shfl. Second,
>> they replace uses of cuda.syncthreads with nvvm.barrier0. Third, all of
>> the ptx intrinsics and builtins are removed. Finally, I wrap the nvvm
>> intrinsics with a TargetPrefix.
>>
>> Let me know if you want me to break any of these out into specific
>> review threads on -commits.
>>
>
>
> -----------------------------------------------------------------------------------
> This email message is for the sole use of the intended recipient(s) and may contain
> confidential information.  Any unauthorized review, use, disclosure or distribution
> is prohibited.  If you are not the intended recipient, please contact the sender by
> reply email and destroy all copies of the original message.
> -----------------------------------------------------------------------------------