[PATCH] D140560: AMDGPU: Fix broken opaque pointer handling in printf pass

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added a comment.

In D140560#4040447 <https://reviews.llvm.org/D140560#4040447>, @nemanjai wrote:

> In D140560#4036217 <https://reviews.llvm.org/D140560#4036217>, @saghir wrote:
>
>> In D140560#4032567 <https://reviews.llvm.org/D140560#4032567>, @arsenm wrote:
>>
>>> In D140560#4031989 <https://reviews.llvm.org/D140560#4031989>, @arsenm wrote:
>>>
>>>> In D140560#4030857 <https://reviews.llvm.org/D140560#4030857>, @saghir wrote:
>>>>
>>>>> Hi, this seems to be causing our bots to fail. One example is https://lab.llvm.org/buildbot/#/builders/231/builds/6915. Can you please take a look? Thanks!
>>>>
>>>> I have a patch ready for this, need to write some more tests for it
>>>
>>> Try 40078a6b713730ffc164d4c0733d26352eb1e236 <https://reviews.llvm.org/rG40078a6b713730ffc164d4c0733d26352eb1e236>
>>
>> Thanks for taking a look. Your patch seems to have fixed the error seen before but it fails at a later check now:
>>
>>   /llvm/test/CodeGen/AMDGPU/opencl-printf.ll:1874:13: error: GCN-NEXT: expected string not found in input
>>   ; GCN-NEXT: store i32 1953722977, ptr addrspace(1) [[PRINTBUFFPTRCAST]], align 4
>>
>> Logs can be found here: https://lab.llvm.org/buildbot/#/builders/231/builds/7035/steps/6/logs/FAIL__LLVM__opencl-printf_ll
>
> Since this is still causing failures (i.e. `DataExtractor` does not reorder bytes for `getBytes()` as it does for `getU...()`), we can fix the problem as follows:
>
>   diff --git a/llvm/lib/Target/AMDGPU/AMDGPUPrintfRuntimeBinding.cpp b/llvm/lib/Target/AMDGPU/AMDGPUPrintfRuntimeBinding.cpp
>   index 18dbf5d..63a365e 100644
>   --- a/llvm/lib/Target/AMDGPU/AMDGPUPrintfRuntimeBinding.cpp
>   +++ b/llvm/lib/Target/AMDGPU/AMDGPUPrintfRuntimeBinding.cpp
>   @@ -435,19 +435,31 @@ bool AMDGPUPrintfRuntimeBindingImpl::lowerPrintfForGpu(Module &M) {
>                DataExtractor Extractor(S, /*IsLittleEndian=*/true, 8);
>                DataExtractor::Cursor Offset(0);
>                while (Offset && Offset.tell() < S.size()) {
>   -              StringRef ReadBytes = Extractor.getBytes(
>   -                  Offset, std::min(ReadSize, S.size() - Offset.tell()));
>   +              uint64_t ReadNow = std::min(ReadSize, S.size() - Offset.tell());
>   +              uint64_t ReadBytes = 0;
>   +              switch (ReadNow) {
>   +              default: llvm_unreachable("min(4, X) > 4?");
>   +              case 1:
>   +                ReadBytes = Extractor.getU8(Offset);
>   +                break;
>   +              case 2:
>   +                ReadBytes = Extractor.getU16(Offset);
>   +                break;
>   +              case 3:
>   +                ReadBytes = Extractor.getU24(Offset);
>   +                break;
>   +              case 4:
>   +                ReadBytes = Extractor.getU32(Offset);
>   +                break;
>   +              }
>    
>                  cantFail(Offset.takeError(),
>                           "failed to read bytes from constant array");
>    
>   -              APInt IntVal(8 * ReadBytes.size(), 0);
>   -              LoadIntFromMemory(
>   -                  IntVal, reinterpret_cast<const uint8_t *>(ReadBytes.data()),
>   -                  ReadBytes.size());
>   +              APInt IntVal(8 * ReadSize, ReadBytes);
>    
>                  // TODO: Should not bothering aligning up.
>   -              if (ReadBytes.size() < ReadSize)
>   +              if (ReadNow < ReadSize)
>                    IntVal = IntVal.zext(8 * ReadSize);
>    
>                  Type *IntTy = Type::getIntNTy(Ctx, IntVal.getBitWidth());
>
> @arsenm Would a fix similar to this be acceptable?

Yes, I was hoping to extend this to up to 16-bytes at a time but I guess I can figure that out then (we should probably just be emitting a memcpy/strcpy anyway)


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

https://reviews.llvm.org/D140560