[MLIR][OpenMP] Refactoring createTargetData in OMPIRBuilder

[TIFitis]

Summary

Key changes:

• Refactor the createTargetData function to make use of the emitOffloadingArrays and emitOffloadingArraysArgument functions to generate code.
• Added a new emitIfClause helper function to allow handling if clauses in a similar fashion to Clang.
• Updated the MLIR side of code to account for changes to createTargetData.

Depends on D149872

[jdoerfert]

Conceptually, I am very confused but mostly because this patch conflates things.

I would suggest to split this part:

This also refactors how the map clause is processed and separates the mlir and IRBuilder parts of the code similar to Clang.

it that is a better way of doing things.

Then, let's talks about use_device_ptr.
For one, I am not sure which test even shows the lowering.

llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h

1899

Everything in this file (should) come with documentation, including (most) members.

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

4112

Style, also elsewhere. I know it's annoying that MLIR broke with the LLVM style, but that is what it is now.

4183–4193

This is only a last resort. We should not generate stuff that is wrong and is replaced. However, if we cannot find a better way we can talk about it.

Generally, I would have assumed that the value in the region in marked to be a use_dev_ptr/addr and that we would generate code for that as soon as we generate code for the value.

[TIFitis]

Removed use_dev_ptr clause related code. This patch only refactors the map clause prcoessing now.
Addressed reviewer comments.

[TIFitis]

Updated tests. All tests pass now.

[TIFitis]

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

4183–4193

I have tried to explain this a little better in https://reviews.llvm.org/D146648

The region code is generated immediately, its that it still uses the old pointer for it instead of the newly allocated device ptr and this fixes that.

4338

getPointerDereferenceableBytes doesn't fully support Arguments and mostly returns 0.

I am looking at adding support for arguments in a separate patch, in the meantime the tests added in this patch fail because of incorrect @.offload_sizes.

[jdoerfert]

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

4338

That function is not the right one for code generation. The sizes need to come from the type + data layout, and potentially the expression used in the map.

[TIFitis]

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

4099–4112

I think these should just use the opaque ptr type instead of the ArrI8PtrTy it is currently using, and we can also get rid of the BitCast instructions.

However doing so would break the tests in OpenMPIRBuilderTest.cpp as that unit test still uses typed pointers.

AFAIK the consensus is to completely get rid of typed pointers in llvm 17. Should I updated this code to use opaque pointers and let the test fail until it gets converted to opaque as well?

4338

Thanks for letting me know.

Just to clarify, do you mean it should come from the MLIR::Type?

The llvm::Type doesn't let me get much information because of Opaque pointers.

[TIFitis]

Changed how size is calculated. Updated tests.

[TIFitis]

mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp

1360–1365

@jdoerfert Is this way of getting the size correct? It seems to work for basic types and arrays which is what we support for now.

[TIFitis]

Updated unit test.

[TIFitis]

Polite request for reviews. Thanks :)

[TIFitis]

ping for reviews. Thanks :)

[TIFitis]

Updated to use opaque pointers, removed BitCast Insts.

[TIFitis]

ping for reviews. Thanks :)

[kiranchandramohan]

Please update the summary to convey all the changes introduced in the patch.

From the tests, it looks like there is a substantial change in the IR. I was hoping this to be an NFC change.

llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h

1409

Please add the new argument to the documentation above.

mlir/test/Target/LLVMIR/omptarget-llvm.mlir

4–5

Why is the test changed?

[TIFitis]

In D146557#4283149, @kiranchandramohan wrote:

Please update the summary to convey all the changes introduced in the patch.

From the tests, it looks like there is a substantial change in the IR. I was hoping this to be an NFC change.

I have updated the summary highlighting the notable changes along with changes to the generated code.

mlir/test/Target/LLVMIR/omptarget-llvm.mlir

4–5

Moved the map_operand to function parameter to be consistent with other tests. I think it can remain unchanged if you prefer.

[kiranchandramohan]

mlir/test/Target/LLVMIR/omptarget-llvm.mlir

4–5

Consistency can be achieved in a follow-up NFC patch that you can submit without review. If the test can only show the differences of this patch it is easier to review.

[kiranchandramohan]

Would it be possible to break this into a few patches like suggested below or similar?

1. Changed to using inlineConvertOmpRegions to generate target data associated region code inline in same block if possible.

2. Changed to calculating map_operand size from mlir type instead of llvm.

Moved getSizeInBytes function back into OpenACC code as it is no longer shared.

3. Moved most of map_operands related codegen to OMPIRBuilder.

Moved mapper_allocas related codegen to OMPIRBuilder.

4. Changed offload_sizes parameter to a global variable same as offload_maptypes to be more consistent with Clang generated code.

[TIFitis]

Updated tests. Addressed reviewer comments.

[TIFitis]

mlir/test/Target/LLVMIR/omptarget-llvm.mlir

4–5

Thanks for letting me know. I've updated the tests.

[TIFitis]

Cleaned up how IsBegin argument is passed for createTargetData call.

[kiranchandramohan]

In D146557#4292223, @TIFitis wrote:

Cleaned up how IsBegin argument is passed for createTargetData call.

Please submit this directly as an NFC patch.

We should always work towards simple patches that are easy to review. Sometimes a big patch is required to give the full context. But it can then be broken up into small patches for faster review. I have created D149153 for the inlineConvertOmpRegions change.

[TIFitis]

In D146557#4295550, @kiranchandramohan wrote:

In D146557#4292223, @TIFitis wrote:

Cleaned up how IsBegin argument is passed for createTargetData call.

Please submit this directly as an NFC patch.

I have merged it separately.

We should always work towards simple patches that are easy to review. Sometimes a big patch is required to give the full context. But it can then be broken up into small patches for faster review. I have created D149153 for the inlineConvertOmpRegions change.

Thanks for creating your patch. I'll try to make my patches shorter in the future.

Once D149153 is merged I'll rebase this patch.

[TIFitis]

Rebased

[kiranchandramohan]

In D146557#4301955, @TIFitis wrote:

In D146557#4295550, @kiranchandramohan wrote:

In D146557#4292223, @TIFitis wrote:

Cleaned up how IsBegin argument is passed for createTargetData call.

Please submit this directly as an NFC patch.

I have merged it separately.

We should always work towards simple patches that are easy to review. Sometimes a big patch is required to give the full context. But it can then be broken up into small patches for faster review. I have created D149153 for the inlineConvertOmpRegions change.

Thanks for creating your patch. I'll try to make my patches shorter in the future.

Once D149153 is merged I'll rebase this patch.

I have submitted D149153. https://github.com/llvm/llvm-project/commit/1ed522623d95ce7dcf95e711b0f2e3844d2e6be1

[TIFitis]

Rebased

[TIFitis]

@kiranchandramohan I am in the process of submitting a new patch for review which moves the map processing from Clang Codegen to OMPIRBuilder. Once that is moved we can just use that for dealing with the map operands.
Thus putting this patch on hold for now.

[TIFitis]

Done migrating from Clang. This makes use of the clang functions now.

[TIFitis]

Small fix to code comments.

[TIFitis]

Rebased

[TIFitis]

Fix lint messages

[TIFitis]

Try to fix patch application.

[TIFitis]

Moved map processing code from functor to new static function.

[TIFitis]

Renamed from processMapOp to genMapInfos