This is an archive of the discontinued LLVM Phabricator instance.

[mlir][openacc] Conversion of data operand to LLVM IR dialect
ClosedPublic

Authored by clementval on May 10 2021, 7:34 AM.

Details

Summary

Add a conversion pass to convert higher-level type before translation.
This conversion extract meangingful information and pack it into a struct that
the translation (D101504) will be able to understand.

Diff Detail

Event Timeline

clementval created this revision.May 10 2021, 7:34 AM
clementval requested review of this revision.May 10 2021, 7:34 AM

Fix header guard

ftynse requested changes to this revision.May 10 2021, 8:55 AM

Thanks! I think this goes into the right direction. I have a couple of small comments and two concerns:

  • the semantics of the OpenACC data descriptor is not documented, so it's hard to tell if the way it is obtained from a memref descriptor is correct or not; most likely it is not because it shouldn't be using the _allocated_ pointer twice;
  • the conversion pass shouldn't subsume std-to-llvm conversion, unless you can't make it work without it.
mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td
256

Nit: operand data -> data operand

mlir/lib/Conversion/OpenACCToLLVM/OpenACCToLLVM.cpp
85
91–92

This looks redundant. dataOperands presumably already contains the relevant operands, no need to enumerate it and get them again.

98

Nit: SmallVector has a default for the number of stack elements. Keep it unless there's a strong reason to choose a different value.

104

This should use the aligned pointer rather than allocated pointer for data access. The allocated pointer is there for freeing the data and may point up to (alignment - 1) bytes before the data you want to read.

115

In most similar cases, we just return failure when the pattern doesn't apply. This isn't as nice for error reporting (the pass may need to do a pre- or post-check, or report a general "conversion failure"), but makes patterns compose. Somebody will be able to use this pattern and have a separate pattern handling their custom type.

Also note that emitting an error does not return, so the code below is still executed.

119

I would have used op.getResultTypes instead of always using the empty type range, for future-proofness if anything.

147

Ideally, we shouldn't be running "std-to-llvm" conversion along with other conversions. I cleaned up most of these, but not all. This leads to poor composability and encourages monolithic conversion flows that don't play nicely with each other (again, suppose we have a third dialect that wants to convert to OpenACC, it would have to recreate all duplicate all of this file).

Consider configuring ConversionTarget to provide materialization that inserts llvm.mlir.cast on type mismatch instead, then run std-to-llvm as a separate pass.

153

Nit: I'd rather call this allDataOperandsAreConverted or something similar that indicates universal quantification. I'd expect hasSomething to return true if there is at least one element of something, but this returns true if all elements are something.

194

Please add the newline.

This revision now requires changes to proceed.May 10 2021, 8:55 AM
ftynse added inline comments.May 10 2021, 8:57 AM
mlir/lib/Conversion/OpenACCToLLVM/CMakeLists.txt
10

This doesn't look necessary here.

12–13

This neither.

It will be good to have the description of the descriptor you are using in the Summary or in the code.

mlir/lib/Conversion/OpenACCToLLVM/OpenACCToLLVM.cpp
114
clementval marked 11 inline comments as done.

Address review comments

Thanks! I think this goes into the right direction. I have a couple of small comments and two concerns:

  • the semantics of the OpenACC data descriptor is not documented, so it's hard to tell if the way it is obtained from a memref descriptor is correct or not; most likely it is not because it shouldn't be using the _allocated_ pointer twice;
  • the conversion pass shouldn't subsume std-to-llvm conversion, unless you can't make it work without it.

Thanks for the extensive review! I have added information about what kind of information is expected to be hold by the data descriptor. For the composition of passes I guess that if we do not run std-to-llvm together with this pass then we should probably process memref just as any other !llvm.struct since we would not have the relation between memref and the converted !llvm.struct. Or did I miss something?

mlir/lib/Conversion/OpenACCToLLVM/OpenACCToLLVM.cpp
85

Thanks. Don't know what I was trying to say here :-)

91–92

operand and data do not have the same type. In case of memref for example, data is still of type memref while operand has the llvm converted type !llvm.struct<. I thought it was the way to intercept memref conversion and being able to extract the meaningful information while we still know it is coming from a memref. I took this idea from here: mlir/lib/Conversion/SPIRVToLLVM/ConvertLaunchFuncToLLVMCalls.cpp.

114

We might add it later if there is a need. "Unsupported" should be read as "Not supported right now but might get support later".

115

Makes sense. Thanks for the info.

119

The three operations EnterDataOp, ExitDataOp and UpdateOp do not have results so they do not have the getResultTypes() function. At least for now.

147

I fully understand that this lead to poor composability. But if run separately then there is no way to know which !llvm.struct where in fact memref when we run this conversion pass. Or did I miss something?

153

Right, it makes more sense.

ftynse accepted this revision.May 11 2021, 1:29 AM
ftynse added inline comments.
mlir/lib/Conversion/OpenACCToLLVM/OpenACCToLLVM.cpp
91–92

I see. Consider naming them accordingly then, something like originalDataOperand and transformedDataOperand.

119

Nit: you can also use return rewriter.notifyMatchFailure("message") for more detailed information.

147

If you run this pass before -std-to-llvm (most conversions are currently set up so that -std-to-llvm runs last anyway), you can convert

%0 = "memref.producer"() : () -> memref<...>
openacc.enterdata copyin(%0) {
 ...
}

into

%0 = "memref.producer"() : () -> memref<...>
%1 = llvm.mlir.cast %0 : memref<...> to !llvm.struct<normal-descriptor>
%2 = llvm.extractvalue %1[1] : !llvm.struct<normal-descriptor>
...
%42 = llvm.mlir.undef : !llvm.struct<openacc-descriptor>
%43 = llvm.insertvalue %42[0], %2 : !llvm.struct<openacc-descriptor>
...
openacc.enterdata copyin(%43)

Normally, if you use LLVMConversionTarget, you shouldn't even need the to insert the llvm.mlir.cast operation yourself. After std-to-llvm runs, both sides of llvm.mlir.cast will have the same type and it will be folded away.

This revision is now accepted and ready to land.May 11 2021, 1:29 AM
clementval added inline comments.May 11 2021, 7:32 AM
mlir/lib/Conversion/OpenACCToLLVM/OpenACCToLLVM.cpp
147

Ok I see. Thanks a lot for the hint it makes lots of sense to do it this. I will try to set this up.

clementval marked 4 inline comments as done.May 11 2021, 11:59 AM

@ftynse Thanks for the quick and very useful review. I made the conversion independent from "std-to-llvm".

mlir/lib/Conversion/OpenACCToLLVM/OpenACCToLLVM.cpp
147

Thanks for the useful hint. "std-to-llvm" is not run anymore with this conversions.

Make the conversion independent of std-to-llvm. Conversion can now be run before
std-to-llvm and cast op will be folded.

Address other review comments.

This revision was landed with ongoing or failed builds.May 12 2021, 8:34 AM
This revision was automatically updated to reflect the committed changes.
rriddle added inline comments.May 12 2021, 2:33 PM
mlir/lib/Conversion/OpenACCToLLVM/OpenACCToLLVM.cpp
75

This class should be in an anonymous namespace.