This is an archive of the discontinued LLVM Phabricator instance.

[mlir][LLVMIR] "Modernize" Insert/ExtractValueOp
ClosedPublic

Authored by Mogball on Aug 9 2022, 8:01 PM.
Tokens
"Like" token, awarded by bzcheeseman.

Details

Summary

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.

Diff Detail

Event Timeline

Mogball created this revision.Aug 9 2022, 8:01 PM
Herald added a reviewer: aartbik. · View Herald Transcript
Herald added a reviewer: ftynse. · View Herald Transcript
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
Mogball requested review of this revision.Aug 9 2022, 8:01 PM

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?

ftynse accepted this revision.Aug 10 2022, 5:09 AM
This revision is now accepted and ready to land.Aug 10 2022, 5:09 AM
Mogball updated this revision to Diff 451530.Aug 10 2022, 9:50 AM
Mogball marked 5 inline comments as done.

review comments

This revision was landed with ongoing or failed builds.Aug 10 2022, 9:51 AM
This revision was automatically updated to reflect the committed changes.
bondhugula added a subscriber: bondhugula.EditedAug 10 2022, 5:09 PM

@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?

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?

I'll fix it.

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?

I'll fix it.

https://reviews.llvm.org/D131637