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

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai added a comment.

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?


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

https://reviews.llvm.org/D140560