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

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUPrintfRuntimeBinding.cpp:333
     Type *I8Ty = Type::getInt8Ty(Ctx);
     Type *I8Ptr = PointerType::get(I8Ty, 1);
     FunctionType *FTy_alloc = FunctionType::get(I8Ptr, Tys_alloc, false);
----------------
Should use GLOBAL_ADDRESS, this is repeated several other times in this function


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUPrintfRuntimeBinding.cpp:384-385
     Type *idPointer = PointerType::get(I32Ty, AMDGPUAS::GLOBAL_ADDRESS);
     Value *id_gep_cast =
         new BitCastInst(BufferIdx, idPointer, "PrintBuffIdCast", Brnch);
 
----------------
Should be using IRBuilder, this is unconditionally creating no-op bitcasts with opaque pointers


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUPrintfRuntimeBinding.cpp:387
 
     new StoreInst(ConstantInt::get(I32Ty, UniqID), id_gep_cast, Brnch);
 
----------------
Should create stores with explicit alignments


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUPrintfRuntimeBinding.cpp:420
           }
         }
         Arg = new BitCastInst(Arg, IType, "PrintArgFP", Brnch);
----------------
None of these paths are tested and I think this is broken for undef/poison. Also need some half handling


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUPrintfRuntimeBinding.cpp:426-435
           if (auto *ConstExpr = dyn_cast<ConstantExpr>(Arg)) {
             auto *GV = dyn_cast<GlobalVariable>(ConstExpr->getOperand(0));
             if (GV && GV->hasInitializer()) {
               Constant *Init = GV->getInitializer();
               bool IsZeroValue = Init->isZeroValue();
               auto *CA = dyn_cast<ConstantDataArray>(Init);
               if (IsZeroValue || (CA && CA->isString())) {
----------------
This is all wrong for the same reasons as the format string


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUPrintfRuntimeBinding.cpp:446
           if (S[0]) {
             char *MyNewStr = new char[NSizeStr]();
             strcpy(MyNewStr, S);
----------------
No raw news. Use SmallString


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUPrintfRuntimeBinding.cpp:467
           Type *DstType = (Size == 32) ? Int32Ty : Int64Ty;
           Arg = new PtrToIntInst(Arg, DstType, "PrintArgPtr", Brnch);
           WhatToStore.push_back(Arg);
----------------
Don't see why we need to use ptrtoint, store of pointer works just as well


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUPrintfRuntimeBinding.cpp:472
         Type *IType = nullptr;
         uint32_t EleCount = cast<FixedVectorType>(ArgType)->getNumElements();
         uint32_t EleSize = ArgType->getScalarSizeInBits();
----------------
Should at least not die on scalable vector IR


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUPrintfRuntimeBinding.cpp:473
         uint32_t EleCount = cast<FixedVectorType>(ArgType)->getNumElements();
         uint32_t EleSize = ArgType->getScalarSizeInBits();
         uint32_t TotalSize = EleCount * EleSize;
----------------
This is broken for vectors of pointers


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUPrintfRuntimeBinding.cpp:520
         Value *TheBtCast = WhatToStore[I];
         unsigned ArgSize = TD->getTypeAllocSizeInBits(TheBtCast->getType()) / 8;
         SmallVector<Value *, 1> BuffOffset;
----------------
Should use getTypeStoreSize


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUPrintfRuntimeBinding.cpp:525-526
         Type *ArgPointer = PointerType::get(TheBtCast->getType(), 1);
         Value *CastedGEP =
             new BitCastInst(BufferIdx, ArgPointer, "PrintBuffPtrCast", Brnch);
         StoreInst *StBuff = new StoreInst(TheBtCast, CastedGEP, Brnch);
----------------
Another unneeded bitcast with opaque pointers


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

https://reviews.llvm.org/D140560