This patch "modernizes" the LLVM insertvalue and extractvalue
operations to use DenseI64ArrayAttr, since they only require an array of
indices and previously there was confusion about whether to use i32 or
i64 arrays, and to use assembly format.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Thanks for the cleanup!
mlir/lib/Conversion/SPIRVToLLVM/SPIRVToLLVM.cpp | ||
---|---|---|
581 | Nit: can we reserve before pushing back in a loop? Same below. | |
mlir/lib/Conversion/VectorToLLVM/ConvertVectorToLLVM.cpp | ||
678 | Nit: expand auto, here and below when there's no cast on the RHS. | |
mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp | ||
1409–1414 | Nit: the "wrapped LLVM IR type" wording doesn't make sense anymore. It refers to the time when LLVM dialect types were literally pointers to LLVM IR types. | |
mlir/lib/Target/LLVMIR/Dialect/LLVMIR/LLVMToLLVMIRTranslation.cpp | ||
263 | Nit: reserve before inserting in the loop. | |
267 | Nit: I've seen a lot of similar patterns above, e.g., in ConvertVectorToLLVM.cpp, would it make sense to provide lift this function as utility available in LLVMDialect.h? |
@Mogball Looks like your revision has broken the build:
/home/uday/llvm-project-upstream/mlir/lib/Conversion/LLVMCommon/MemRefBuilder.cpp: In member function ‘mlir::Value mlir::MemRefDescriptor::stride(mlir::OpBuilder&, mlir::Location, unsigned int)’: /home/uday/llvm-project-upstream/mlir/lib/Conversion/LLVMCommon/MemRefBuilder.cpp:170:70: error: call of overloaded ‘makeArrayRef<int64_t>(<brace-enclosed initializer list>)’ is ambiguous llvm::makeArrayRef<int64_t>({kStridePosInMemRefDescriptor, pos})); ^ In file included from /home/uday/llvm-project-upstream/llvm/include/llvm/ADT/STLExtras.h:20, from /home/uday/llvm-project-upstream/mlir/include/mlir/Support/TypeID.h:20, from /home/uday/llvm-project-upstream/mlir/include/mlir/IR/MLIRContext.h:13, from /home/uday/llvm-project-upstream/mlir/include/mlir/IR/TypeSupport.h:16, from /home/uday/llvm-project-upstream/mlir/include/mlir/IR/Types.h:12, from /home/uday/llvm-project-upstream/mlir/include/mlir/Conversion/LLVMCommon/StructBuilder.h:17, from /home/uday/llvm-project-upstream/mlir/include/mlir/Conversion/LLVMCommon/MemRefBuilder.h:17, from /home/uday/llvm-project-upstream/mlir/lib/Conversion/LLVMCommon/MemRefBuilder.cpp:9: /home/uday/llvm-project-upstream/llvm/include/llvm/ADT/ArrayRef.h:505:15: note: candidate: ‘llvm::ArrayRef<T> llvm::makeArrayRef(const std::vector<T>&) [with T = long int]’ ArrayRef<T> makeArrayRef(const std::vector<T> &Vec) { ^~~~~~~~~~~~ /home/uday/llvm-project-upstream/llvm/include/llvm/ADT/ArrayRef.h:516:37: note: candidate: ‘llvm::ArrayRef<T> llvm::makeArrayRef(const llvm::ArrayRef<T>&) [with T = long int]’ template <typename T> ArrayRef<T> makeArrayRef(const ArrayRef<T> &Vec) { ^
gcc --version gcc (GCC) 8.5.0 20210514 (Red Hat 8.5.0-13)
Hm, that's annoying. I didn't get an email from the buildbots. I'll make a fix but would you be able to test it for me?
Nit: can we reserve before pushing back in a loop? Same below.