This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Turn flags in ConvertStandardToLLVM into pass flags
ClosedPublic

Authored by dcaballe on Feb 3 2020, 11:21 AM.

Details

Summary

Follow-up on D72802. Turn -convert-std-to-llvm-use-alloca and
-convert-std-to-llvm-bare-ptr-memref-call-conv into pass flags
of LLVMLoweringPass.

Diff Detail

Unit TestsFailed

Event Timeline

dcaballe created this revision.Feb 3 2020, 11:21 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 3 2020, 11:21 AM

Unit tests: fail. 62298 tests passed, 121 failed and 845 were skipped.

failed: Clang.CXX/modules-ts/basic/basic_link/p3.cppm
failed: Clang.CXX/modules-ts/codegen-basics.cppm
failed: Clang.CodeGen/arm-aapcs-vfp.c
failed: Clang.CodeGen/attr-cpuspecific.c
failed: Clang.CodeGen/attr-target-mv-func-ptrs.c
failed: Clang.CodeGen/attr-target-mv-va-args.c
failed: Clang.CodeGenCUDA/address-spaces.cu
failed: Clang.CodeGenCUDA/amdgpu-kernel-arg-pointer-type.cu
failed: Clang.CodeGenCUDA/amdgpu-kernel-attrs.cu
failed: Clang.CodeGenCUDA/amdgpu-visibility.cu
failed: Clang.CodeGenCUDA/convergent.cu
failed: Clang.CodeGenCUDA/cuda-builtin-vars.cu
failed: Clang.CodeGenCUDA/device-stub.cu
failed: Clang.CodeGenCUDA/device-var-init.cu
failed: Clang.CodeGenCUDA/device-vtable.cu
failed: Clang.CodeGenCUDA/function-overload.cu
failed: Clang.CodeGenCUDA/kernel-amdgcn.cu
failed: Clang.CodeGenCUDA/kernel-args.cu
failed: Clang.CodeGenCUDA/library-builtin.cu
failed: Clang.CodeGenCUDA/link-device-bitcode.cu
failed: Clang.CodeGenCUDA/nothrow.cu
failed: Clang.CodeGenCUDA/propagate-metadata.cu
failed: Clang.CodeGenCUDA/ptx-kernels.cu
failed: Clang.CodeGenCUDA/types.cu
failed: Clang.CodeGenCUDA/usual-deallocators.cu
failed: Clang.CodeGenCXX/attr-exclude_from_explicit_instantiation.dont_assume_extern_instantiation.cpp
failed: Clang.CodeGenCXX/attr-target-mv-diff-ns.cpp
failed: Clang.CodeGenCXX/attr-target-mv-member-funcs.cpp
failed: Clang.CodeGenCXX/attr-target-mv-out-of-line-defs.cpp
failed: Clang.CodeGenCXX/attr-target-mv-overloads.cpp
failed: Clang.CodeGenCXX/attr.cpp
failed: Clang.CodeGenCXX/builtin-source-location.cpp
failed: Clang.CodeGenCXX/conditional-temporaries.cpp
failed: Clang.CodeGenCXX/const-init-cxx2a.cpp
failed: Clang.CodeGenCXX/ctor-dtor-alias.cpp
failed: Clang.CodeGenCXX/cxx0x-initializer-stdinitializerlist-pr12086.cpp
failed: Clang.CodeGenCXX/cxx0x-initializer-stdinitializerlist-startend.cpp
failed: Clang.CodeGenCXX/cxx0x-initializer-stdinitializerlist.cpp
failed: Clang.CodeGenCXX/cxx11-extern-constexpr.cpp
failed: Clang.CodeGenCXX/cxx11-initializer-aggregate.cpp
failed: Clang.CodeGenCXX/cxx11-thread-local-reference.cpp
failed: Clang.CodeGenCXX/cxx11-thread-local-visibility.cpp
failed: Clang.CodeGenCXX/cxx1y-variable-template-linkage.cpp
failed: Clang.CodeGenCXX/cxx1z-inline-variables.cpp
failed: Clang.CodeGenCXX/explicit-instantiation.cpp
failed: Clang.CodeGenCXX/inheriting-constructor.cpp
failed: Clang.CodeGenCXX/member-function-pointers.cpp
failed: Clang.CodeGenCXX/microsoft-uuidof.cpp
failed: Clang.CodeGenCXX/no-odr-use.cpp
failed: Clang.CodeGenCXX/pragma-visibility.cpp
failed: Clang.CodeGenCXX/regcall.cpp
failed: Clang.CodeGenCXX/static-data-member.cpp
failed: Clang.CodeGenCXX/static-init.cpp
failed: Clang.CodeGenCXX/static-member-variable-explicit-specialization.cpp
failed: Clang.CodeGenCXX/visibility-inlines-hidden-staticvar.cpp
failed: Clang.CodeGenCXX/vtable-linkage.cpp
failed: Clang.CodeGenCXX/x86_64-arguments.cpp
failed: Clang.CodeGenCoroutines/coro-await-resume-eh.cpp
failed: Clang.CodeGenCoroutines/coro-await.cpp
failed: Clang.CodeGenCoroutines/coro-cleanup.cpp
failed: Clang.CodeGenCoroutines/coro-gro-nrvo.cpp
failed: Clang.CodeGenCoroutines/coro-params.cpp
failed: Clang.CodeGenCoroutines/coro-ret-void.cpp
failed: Clang.CodeGenObjC/assign.m
failed: Clang.CodeGenObjC/constant-strings.m
failed: Clang.CodeGenObjC/gnu-exceptions.m
failed: Clang.CodeGenObjC/gnustep2-proto.m
failed: Clang.CodeGenObjC/objfw.m
failed: Clang.CodeGenObjC/property.m
failed: Clang.CodeGenObjC/stret_lookup.m
failed: Clang.CodeGenObjCXX/designated-initializers.mm
failed: Clang.CodeGenObjCXX/objfw-exceptions.mm
failed: Clang.CodeGenOpenCL/addr-space-struct-arg.cl
failed: Clang.CodeGenOpenCL/address-spaces-conversions.cl
failed: Clang.CodeGenOpenCL/amdgcn-automatic-variable.cl
failed: Clang.CodeGenOpenCL/amdgcn-large-globals.cl
failed: Clang.CodeGenOpenCL/amdgpu-abi-struct-coerce.cl
failed: Clang.CodeGenOpenCL/amdgpu-attrs.cl
failed: Clang.CodeGenOpenCL/amdgpu-call-kernel.cl
failed: Clang.CodeGenOpenCL/amdgpu-calling-conv.cl
failed: Clang.CodeGenOpenCL/amdgpu-enqueue-kernel.cl
failed: Clang.CodeGenOpenCL/amdgpu-nullptr.cl
failed: Clang.CodeGenOpenCL/as_type.cl
failed: Clang.CodeGenOpenCL/bool_cast.cl
failed: Clang.CodeGenOpenCL/cl20-device-side-enqueue.cl
failed: Clang.CodeGenOpenCL/constant-addr-space-globals.cl
failed: Clang.CodeGenOpenCL/convergent.cl
failed: Clang.CodeGenOpenCL/extension-begin.cl
failed: Clang.CodeGenOpenCL/kernel-arg-info.cl
failed: Clang.CodeGenOpenCL/kernels-have-spir-cc-by-default.cl
failed: Clang.CodeGenOpenCL/partial_initializer.cl
failed: Clang.CodeGenOpenCL/pipe_types.cl
failed: Clang.CodeGenOpenCL/ptx-calls.cl
failed: Clang.CodeGenOpenCL/ptx-kernels.cl
failed: Clang.CodeGenOpenCL/sampler.cl
failed: Clang.CodeGenOpenCL/shifts.cl
failed: Clang.CodeGenOpenCL/spir-calling-conv.cl
failed: Clang.CodeGenOpenCL/vectorLoadStore.cl
failed: Clang.CodeGenOpenCL/visibility.cl
failed: Clang.CodeGenOpenCL/vla.cl
failed: Clang.CodeGenOpenCLCXX/address-space-deduction.cl
failed: Clang.CodeGenOpenCLCXX/addrspace-derived-base.cl
failed: Clang.CodeGenOpenCLCXX/addrspace-operators.cl
failed: Clang.CodeGenOpenCLCXX/addrspace-references.cl
failed: Clang.CodeGenOpenCLCXX/addrspace-with-class.cl
failed: Clang.CodeGenOpenCLCXX/constexpr.cl
failed: Clang.Modules/codegen-flags.test
failed: Clang.Modules/codegen-opt.test
failed: Clang.Modules/codegen.test
failed: Clang.Modules/initializers.cpp
failed: Clang.Modules/templates-2.mm
failed: Clang.Modules/templates.mm
failed: Clang.OpenMP/allocate_codegen.cpp
failed: Clang.OpenMP/declare_variant_mixed_codegen.c
failed: Clang.OpenMP/parallel_codegen.cpp
failed: Clang.OpenMP/parallel_master_codegen.cpp
failed: Clang.OpenMP/simd_metadata.c
failed: Clang.OpenMP/threadprivate_codegen.cpp
failed: Clang.PCH/codegen.cpp
failed: Clang.Parser/pragma-visibility2.c
failed: Clang.Profile/cxx-abc-deleting-dtor.cpp

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

flaub added a subscriber: flaub.Feb 3 2020, 12:09 PM
flaub added inline comments.
mlir/include/mlir/Conversion/StandardToLLVM/ConvertStandardToLLVMPass.h
61–62

When building a pipeline at runtime (that is, not using the command line parsing), it'd be good to be able to specify options during pass creation. I've noticed that currently, even the useAlloca bool wasn't actually being used, only the clUseAlloca was used. So this is a step in the right direction (using instance specific options rather than global ones), but I think we still need the ability to pass options to the creation function.

rriddle added inline comments.Feb 3 2020, 12:23 PM
mlir/include/mlir/Conversion/StandardToLLVM/ConvertStandardToLLVMPass.h
61–62

+1

mlir/lib/Conversion/StandardToLLVM/ConvertStandardToLLVM.cpp
2784–2785

You can remove this functor now.

ftynse added inline comments.Feb 3 2020, 1:19 PM
mlir/include/mlir/Conversion/StandardToLLVM/ConvertStandardToLLVMPass.h
61–62

I've noticed that currently, even the useAlloca bool wasn't actually being used

I fixed that in e0ea706a59b9032b7f3590478080adf4f3e1486a.

+1 on being able to specify flags at pass creation time.

dcaballe updated this revision to Diff 242207.Feb 3 2020, 3:06 PM
dcaballe marked 4 inline comments as done.
Thanks! Addressing feedback.
dcaballe planned changes to this revision.Feb 3 2020, 3:09 PM

Sorry, missed one piece.

Unit tests: fail. 62298 tests passed, 121 failed and 845 were skipped.

failed: Clang.CXX/modules-ts/basic/basic_link/p3.cppm
failed: Clang.CXX/modules-ts/codegen-basics.cppm
failed: Clang.CodeGen/arm-aapcs-vfp.c
failed: Clang.CodeGen/attr-cpuspecific.c
failed: Clang.CodeGen/attr-target-mv-func-ptrs.c
failed: Clang.CodeGen/attr-target-mv-va-args.c
failed: Clang.CodeGenCUDA/address-spaces.cu
failed: Clang.CodeGenCUDA/amdgpu-kernel-arg-pointer-type.cu
failed: Clang.CodeGenCUDA/amdgpu-kernel-attrs.cu
failed: Clang.CodeGenCUDA/amdgpu-visibility.cu
failed: Clang.CodeGenCUDA/convergent.cu
failed: Clang.CodeGenCUDA/cuda-builtin-vars.cu
failed: Clang.CodeGenCUDA/device-stub.cu
failed: Clang.CodeGenCUDA/device-var-init.cu
failed: Clang.CodeGenCUDA/device-vtable.cu
failed: Clang.CodeGenCUDA/function-overload.cu
failed: Clang.CodeGenCUDA/kernel-amdgcn.cu
failed: Clang.CodeGenCUDA/kernel-args.cu
failed: Clang.CodeGenCUDA/library-builtin.cu
failed: Clang.CodeGenCUDA/link-device-bitcode.cu
failed: Clang.CodeGenCUDA/nothrow.cu
failed: Clang.CodeGenCUDA/propagate-metadata.cu
failed: Clang.CodeGenCUDA/ptx-kernels.cu
failed: Clang.CodeGenCUDA/types.cu
failed: Clang.CodeGenCUDA/usual-deallocators.cu
failed: Clang.CodeGenCXX/attr-exclude_from_explicit_instantiation.dont_assume_extern_instantiation.cpp
failed: Clang.CodeGenCXX/attr-target-mv-diff-ns.cpp
failed: Clang.CodeGenCXX/attr-target-mv-member-funcs.cpp
failed: Clang.CodeGenCXX/attr-target-mv-out-of-line-defs.cpp
failed: Clang.CodeGenCXX/attr-target-mv-overloads.cpp
failed: Clang.CodeGenCXX/attr.cpp
failed: Clang.CodeGenCXX/builtin-source-location.cpp
failed: Clang.CodeGenCXX/conditional-temporaries.cpp
failed: Clang.CodeGenCXX/const-init-cxx2a.cpp
failed: Clang.CodeGenCXX/ctor-dtor-alias.cpp
failed: Clang.CodeGenCXX/cxx0x-initializer-stdinitializerlist-pr12086.cpp
failed: Clang.CodeGenCXX/cxx0x-initializer-stdinitializerlist-startend.cpp
failed: Clang.CodeGenCXX/cxx0x-initializer-stdinitializerlist.cpp
failed: Clang.CodeGenCXX/cxx11-extern-constexpr.cpp
failed: Clang.CodeGenCXX/cxx11-initializer-aggregate.cpp
failed: Clang.CodeGenCXX/cxx11-thread-local-reference.cpp
failed: Clang.CodeGenCXX/cxx11-thread-local-visibility.cpp
failed: Clang.CodeGenCXX/cxx1y-variable-template-linkage.cpp
failed: Clang.CodeGenCXX/cxx1z-inline-variables.cpp
failed: Clang.CodeGenCXX/explicit-instantiation.cpp
failed: Clang.CodeGenCXX/inheriting-constructor.cpp
failed: Clang.CodeGenCXX/member-function-pointers.cpp
failed: Clang.CodeGenCXX/microsoft-uuidof.cpp
failed: Clang.CodeGenCXX/no-odr-use.cpp
failed: Clang.CodeGenCXX/pragma-visibility.cpp
failed: Clang.CodeGenCXX/regcall.cpp
failed: Clang.CodeGenCXX/static-data-member.cpp
failed: Clang.CodeGenCXX/static-init.cpp
failed: Clang.CodeGenCXX/static-member-variable-explicit-specialization.cpp
failed: Clang.CodeGenCXX/visibility-inlines-hidden-staticvar.cpp
failed: Clang.CodeGenCXX/vtable-linkage.cpp
failed: Clang.CodeGenCXX/x86_64-arguments.cpp
failed: Clang.CodeGenCoroutines/coro-await-resume-eh.cpp
failed: Clang.CodeGenCoroutines/coro-await.cpp
failed: Clang.CodeGenCoroutines/coro-cleanup.cpp
failed: Clang.CodeGenCoroutines/coro-gro-nrvo.cpp
failed: Clang.CodeGenCoroutines/coro-params.cpp
failed: Clang.CodeGenCoroutines/coro-ret-void.cpp
failed: Clang.CodeGenObjC/assign.m
failed: Clang.CodeGenObjC/constant-strings.m
failed: Clang.CodeGenObjC/gnu-exceptions.m
failed: Clang.CodeGenObjC/gnustep2-proto.m
failed: Clang.CodeGenObjC/objfw.m
failed: Clang.CodeGenObjC/property.m
failed: Clang.CodeGenObjC/stret_lookup.m
failed: Clang.CodeGenObjCXX/designated-initializers.mm
failed: Clang.CodeGenObjCXX/objfw-exceptions.mm
failed: Clang.CodeGenOpenCL/addr-space-struct-arg.cl
failed: Clang.CodeGenOpenCL/address-spaces-conversions.cl
failed: Clang.CodeGenOpenCL/amdgcn-automatic-variable.cl
failed: Clang.CodeGenOpenCL/amdgcn-large-globals.cl
failed: Clang.CodeGenOpenCL/amdgpu-abi-struct-coerce.cl
failed: Clang.CodeGenOpenCL/amdgpu-attrs.cl
failed: Clang.CodeGenOpenCL/amdgpu-call-kernel.cl
failed: Clang.CodeGenOpenCL/amdgpu-calling-conv.cl
failed: Clang.CodeGenOpenCL/amdgpu-enqueue-kernel.cl
failed: Clang.CodeGenOpenCL/amdgpu-nullptr.cl
failed: Clang.CodeGenOpenCL/as_type.cl
failed: Clang.CodeGenOpenCL/bool_cast.cl
failed: Clang.CodeGenOpenCL/cl20-device-side-enqueue.cl
failed: Clang.CodeGenOpenCL/constant-addr-space-globals.cl
failed: Clang.CodeGenOpenCL/convergent.cl
failed: Clang.CodeGenOpenCL/extension-begin.cl
failed: Clang.CodeGenOpenCL/kernel-arg-info.cl
failed: Clang.CodeGenOpenCL/kernels-have-spir-cc-by-default.cl
failed: Clang.CodeGenOpenCL/partial_initializer.cl
failed: Clang.CodeGenOpenCL/pipe_types.cl
failed: Clang.CodeGenOpenCL/ptx-calls.cl
failed: Clang.CodeGenOpenCL/ptx-kernels.cl
failed: Clang.CodeGenOpenCL/sampler.cl
failed: Clang.CodeGenOpenCL/shifts.cl
failed: Clang.CodeGenOpenCL/spir-calling-conv.cl
failed: Clang.CodeGenOpenCL/vectorLoadStore.cl
failed: Clang.CodeGenOpenCL/visibility.cl
failed: Clang.CodeGenOpenCL/vla.cl
failed: Clang.CodeGenOpenCLCXX/address-space-deduction.cl
failed: Clang.CodeGenOpenCLCXX/addrspace-derived-base.cl
failed: Clang.CodeGenOpenCLCXX/addrspace-operators.cl
failed: Clang.CodeGenOpenCLCXX/addrspace-references.cl
failed: Clang.CodeGenOpenCLCXX/addrspace-with-class.cl
failed: Clang.CodeGenOpenCLCXX/constexpr.cl
failed: Clang.Modules/codegen-flags.test
failed: Clang.Modules/codegen-opt.test
failed: Clang.Modules/codegen.test
failed: Clang.Modules/initializers.cpp
failed: Clang.Modules/templates-2.mm
failed: Clang.Modules/templates.mm
failed: Clang.OpenMP/allocate_codegen.cpp
failed: Clang.OpenMP/declare_variant_mixed_codegen.c
failed: Clang.OpenMP/parallel_codegen.cpp
failed: Clang.OpenMP/parallel_master_codegen.cpp
failed: Clang.OpenMP/simd_metadata.c
failed: Clang.OpenMP/threadprivate_codegen.cpp
failed: Clang.PCH/codegen.cpp
failed: Clang.Parser/pragma-visibility2.c
failed: Clang.Profile/cxx-abc-deleting-dtor.cpp

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

dcaballe marked an inline comment as done.Feb 4 2020, 9:43 AM
dcaballe added inline comments.
mlir/include/mlir/Conversion/StandardToLLVM/ConvertStandardToLLVMPass.h
61–62

Thanks for bringing this up! I'm actually having second thoughts on this patch in general since I'm hitting a problem. Before this change, I was able to use these flags at runtime by passing the flags directly through my nGraph command line. This was very handy since I could experiment with different options without introducing changes or adding/replicating flags on the nGraph side (this is also true for other LLVM cl flags). However, it seems that moving the flags from global cl options to pass cl options means that we can only use these flags with mlir-opt since now they are part of the LLVM lowering pass flag (i.e., -convert-std-to-llvm='use-bare-ptr-memref-call-conv=1'), which only makes sense with mlir-opt or similar tools that process pass flags. I haven't found a way to use these flags in the same way as before. Maybe there's something missing on my side. Any thoughts about this?

dcaballe marked an inline comment as done.Feb 10 2020, 10:32 AM
dcaballe added inline comments.
mlir/include/mlir/Conversion/StandardToLLVM/ConvertStandardToLLVMPass.h
61–62

@ftynse, @rriddle, @mehdi_amini, please, let me know what you think about this when you have a chance so that I can move forward. It seems the general consensus is moving global flags to pass flags that will no longer be available externally. However, what happens with those flags that are being used by external clients such as in this case? Should clients duplicate those flags externally? Won't that be too much duplication?

mehdi_amini added inline comments.Feb 10 2020, 11:05 AM
mlir/include/mlir/Conversion/StandardToLLVM/ConvertStandardToLLVMPass.h
61–62

It seems the general consensus is moving global flags to pass flags

Yes: you can consider this a hard policy in MLIR from my point of view.

that will no longer be available externally.

Why wouldn't the option be exposed in the API for creating the pass? Options are really part of the API to m.

However, what happens with those flags that are being used by external clients such as in this case? Should clients duplicate those flags externally? Won't that be too much duplication?

I don't know what you mean by "duplication": like every API you're interacting with, you need to pass parameters in. The way you're getting this information in the first place is up to the client, maybe you want to expose this to the user with a command line flag, or maybe you want to select a particular configuration for your target.

Putting options in cl::opt is *exclusively* for debugging, these are just not an API.
And we should limit these debug flags as much as possible IMO: there is never a reason to use them directly in a pass (pass options should fit all the cases), which leave them for cases deep in libraries where threaded them up to the APIs would be too cumbersome compared to the intended usage.

dcaballe marked an inline comment as done.Feb 10 2020, 3:28 PM
dcaballe added inline comments.
mlir/include/mlir/Conversion/StandardToLLVM/ConvertStandardToLLVMPass.h
61–62

Thanks, Mehdi! Don't get me wrong. I agree with the general direction. However, I think there is a gap in functionality. By duplication I meant clients adding the same equivalent flag to toggle a feature that is not on the client side but on the core MLIR side. As a client, I'd like to have my own set of flags but also to be able to forward flags to my internal components when those flags don't impact my code base (something similar to what Clang does with the -mllvm flag). For clients interacting with multiple internal components, having to duplicate all the flags of the internal components can be a big deal. I wonder if we could find a way to explicitly forward or selectively expose pass flags to external clients. In any case, a separate discussion. I'll update this patch shortly. Thanks!

dcaballe updated this revision to Diff 243697.Feb 10 2020, 4:42 PM

Rebase + move 'emit-c-wrappers' flag to pass

dcaballe marked an inline comment as done.Feb 10 2020, 4:46 PM

Ready

mlir/lib/Conversion/StandardToLLVM/ConvertStandardToLLVM.cpp
48

don't we have any tests exercising this flag?

mehdi_amini added inline comments.Feb 10 2020, 8:32 PM
mlir/include/mlir/Conversion/StandardToLLVM/ConvertStandardToLLVMPass.h
61–62

what Clang does with the -mllvm flag

Note this is purely intended for debugging.

explicitly forward or selectively expose pass flags to external clients

A problem is collision: we need to handle the fact that two passes would both define a "threshold" option for instance.

You could get around this by having this somehow handled by your pipeline builder, taking something like:

-mlir-pass-option=pass1{threshold=2},pass2{threshold=1}

Then you can split the string and associate pass1 with the {threshold=2} string and use this to build the pass.
Note that this is still not fully general as you may have a pass multiple times in the pipeline...

In the end there is no silver bullet: LLVM has a convenient mechanism based on globals and global registrations, this is nice for playing but quickly annoying when you have to integrate it as a library in a larger product. It causes issues with reproducibility and has limitation around running the same pass multiple times with different options.

mehdi_amini accepted this revision.Feb 10 2020, 8:38 PM
This revision is now accepted and ready to land.Feb 10 2020, 8:38 PM
This revision was automatically updated to reflect the committed changes.