Page MenuHomePhabricator

[mlir][Vector] Add lowering to LLVM for vector.bitcast
ClosedPublic

Authored by dcaballe on Jan 27 2021, 5:56 PM.

Diff Detail

Event Timeline

dcaballe created this revision.Jan 27 2021, 5:56 PM
dcaballe requested review of this revision.Jan 27 2021, 5:56 PM
ThomasRaoux added inline comments.Jan 27 2021, 6:37 PM
mlir/lib/Conversion/VectorToLLVM/ConvertVectorToLLVM.cpp
293–294

This only works for vector of rank 1 right? We should probably check and fail for larger ranks?

mlir/test/Conversion/VectorToLLVM/vector-to-llvm.mlir
36

I don't think this does anything if you don't add -split-input-file to the command line. Also I'm not sure you need it for this file?

dcaballe updated this revision to Diff 319761.Jan 27 2021, 9:40 PM
dcaballe marked an inline comment as done.

Addressing feedback

mlir/lib/Conversion/VectorToLLVM/ConvertVectorToLLVM.cpp
293–294

Good catch, thanks!

mlir/test/Conversion/VectorToLLVM/vector-to-llvm.mlir
36

Sorry, I missed the flag. I thought we were modernizing some of these old tests and adding -split-input-file to them because there was some benefit in doing so. Isn't that the case? Isn't each chunk processed in parallel? (Probably not because lit tests already run in parallel...). Please, let me know! I would be interested in knowing the actual purpose of this flag! I'll remove it if there is no actual benefit. Thanks!

mehdi_amini added inline comments.Jan 27 2021, 9:56 PM
mlir/test/Conversion/VectorToLLVM/vector-to-llvm.mlir
36

This flag will make each function processed in an isolated module.
This is useful in many cases:

  • expected failures: you want to check diagnostics, but you still want to check all the functions and not stop on the first error
  • you can reuse the same name for multiple functions in the same file
  • some test are sensitive to the environment in the module, that make each "conceptual test" in the file "isolated" from each other.
  • a new context will be used ensure no "pollution" of the environment in between each split "test"

Any other comments?

mlir/test/Conversion/VectorToLLVM/vector-to-llvm.mlir
36

Thanks, Mehdi.

ThomasRaoux accepted this revision.Feb 1 2021, 10:16 AM

Looks good!

This revision is now accepted and ready to land.Feb 1 2021, 10:16 AM
aartbik accepted this revision.Feb 1 2021, 12:10 PM
This revision was automatically updated to reflect the committed changes.