[PATCH] D140560: AMDGPU: Fix broken opaque pointer handling in printf pass
Nemanja Ivanovic via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Jan 10 09:45:39 PST 2023
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
More information about the llvm-commits
mailing list