This is an archive of the discontinued LLVM Phabricator instance.

[MLIR][OpenMP] Refactoring createTargetData in OMPIRBuilder
ClosedPublic

Authored by TIFitis on Mar 21 2023, 12:18 PM.

Details

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

Diff Detail

Event Timeline

TIFitis created this revision.Mar 21 2023, 12:18 PM
Herald added a project: Restricted Project. · View Herald Transcript
TIFitis requested review of this revision.Mar 21 2023, 12:18 PM
Herald added projects: Restricted Project, Restricted Project, Restricted Project. · View Herald Transcript
TIFitis added inline comments.Mar 21 2023, 12:28 PM
llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
4153–4163

I couldn't come up with a better way for the code generated inside the target data region to use the new AllocaPtrs when accessing map operands marked with the use_device_ptr clause.

Currently I am just comparing the Users list before and after generating the data region and replacing the new uses.

Alternative method would be to use mlir::LLVM::moduleTranslation.mapValue method to temporarily remap the mlir::Value for the use_device_ptr operands to the AllocaPtrs llvm::Value. But mapValue doesn't allow remapping of values. @kiranchandramohan Is there a good reason to prevent remapping of values?

Let me know if anyone has a better way of doing this :)

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
1815

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

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
4076

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

4153–4163

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 updated this revision to Diff 507337.Mar 22 2023, 6:16 AM

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

TIFitis retitled this revision from [MLIR][OpenMP] Added OMPIRBuilder support for use_device_ptr clause to [MLIR][OpenMP] Refactoring how map clause is processed.Mar 22 2023, 6:17 AM
TIFitis edited the summary of this revision. (Show Details)
TIFitis updated this revision to Diff 507430.Mar 22 2023, 11:00 AM

Updated tests. All tests pass now.

TIFitis marked 3 inline comments as done.Mar 22 2023, 11:43 AM
TIFitis added inline comments.
llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
4153–4163

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.

4220

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 added inline comments.Mar 22 2023, 2:45 PM
llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
4220

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 marked 2 inline comments as done.Mar 23 2023, 7:00 AM
TIFitis added inline comments.
llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
4063–4076

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?

4220

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 updated this revision to Diff 509698.Mar 30 2023, 9:20 AM
TIFitis marked an inline comment as done.

Changed how size is calculated. Updated tests.

TIFitis added inline comments.Mar 30 2023, 9:25 AM
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
1357–1362

@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 this revision to Diff 509725.Mar 30 2023, 10:45 AM

Updated unit test.

TIFitis marked 2 inline comments as not done.Apr 3 2023, 4:28 AM

Polite request for reviews. Thanks :)

ping for reviews. Thanks :)

TIFitis updated this revision to Diff 513599.Apr 14 2023, 7:59 AM

Updated to use opaque pointers, removed BitCast Insts.

ping for reviews. Thanks :)

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
1383

Please add the new argument to the documentation above.

mlir/test/Target/LLVMIR/omptarget-llvm.mlir
4–5

Why is the test changed?

TIFitis edited the summary of this revision. (Show Details)Apr 20 2023, 5:24 AM

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.

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.

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.
  1. 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.

  1. Moved most of map_operands related codegen to OMPIRBuilder.

Moved mapper_allocas related codegen to OMPIRBuilder.

  1. Changed offload_sizes parameter to a global variable same as offload_maptypes to be more consistent with Clang generated code.
TIFitis updated this revision to Diff 515805.Apr 21 2023, 9:44 AM
TIFitis marked an inline comment as done.
TIFitis edited the summary of this revision. (Show Details)

Updated tests. Addressed reviewer comments.

TIFitis marked 2 inline comments as done.Apr 21 2023, 9:46 AM
TIFitis added inline comments.
mlir/test/Target/LLVMIR/omptarget-llvm.mlir
4–5

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

TIFitis updated this revision to Diff 516379.Apr 24 2023, 6:13 AM
TIFitis marked an inline comment as done.

Cleaned up how IsBegin argument is passed for createTargetData call.

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.

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.

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 edited the summary of this revision. (Show Details)Apr 28 2023, 7:46 AM
TIFitis planned changes to this revision.Apr 28 2023, 7:49 AM

@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 updated this revision to Diff 523303.May 18 2023, 2:54 AM

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

TIFitis retitled this revision from [MLIR][OpenMP] Refactoring how map clause is processed to [MLIR][OpenMP] Refactoring createTargetData in OMPIRBuilder.May 18 2023, 2:55 AM
TIFitis edited the summary of this revision. (Show Details)
TIFitis updated this revision to Diff 523304.May 18 2023, 2:57 AM
TIFitis edited the summary of this revision. (Show Details)

Small fix to code comments.

TIFitis updated this revision to Diff 523318.May 18 2023, 3:33 AM

Fix lint messages

TIFitis updated this revision to Diff 523331.May 18 2023, 4:08 AM

Try to fix patch application.

TIFitis updated this revision to Diff 524301.May 22 2023, 7:38 AM

Moved map processing code from functor to new static function.

TIFitis updated this revision to Diff 524333.May 22 2023, 8:43 AM

Renamed from processMapOp to genMapInfos

jdoerfert added inline comments.Jun 9 2023, 10:32 AM
llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
1357

why are these capitalized?

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
4115

Can we not use an int for some "type". The 1 and 2 below are magic, use an enum and descriptive words.

4139

No llvm::. Also elsewhere.

4363

This function doesn't make sense to me. For one, I don't know what a "unreal block" is. Nor would I have expected a block with terminator to be silently not touched. It might just be a documentation issue (in the header). I would avoid duplicating the comment here again.

4421

CFG Utils have helpers for these things. Do we not use them on purpose?

TIFitis updated this revision to Diff 530518.Jun 12 2023, 8:31 AM
TIFitis marked 5 inline comments as done and an inline comment as not done.

Addressed reviewer comments.

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
4363

I've removed the comment here and updated it in the header file.

The current comment was taken directly from Clang. I've updated it say exactly what it does.

4421

If you are pointing at the SplitBlockAndInsertIfThen like functions in llvm/lib/Transforms/Utils/BasicBlockUtils.cpp then they require the current BB to be well formed with a terminator which it is not in this case.

Earlier we were adding an Unreachable inst and removing it later, however this function I think does a much cleaner job of handling all the cases including region code gen.

Also, it is not apparent in this patch but future patches including some from Jan in review make use of the emitBlock and emitBranch functions directly.

jdoerfert added inline comments.Jun 12 2023, 10:57 AM
llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
1356

This does not mention the deletion stuff, etc.

1803

documentation, what do they mean

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
4180

Style, also elsewhere.

4370

you should not just delete BB. eraseFromParent is a better way. That said, it is also weird you would delete something in an "emit" function.

4387–4389
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
1357–1362

I don't know about mlir, if the DL has a "store size" use that.
That said, the size should potentially come from the user/front-end.

TIFitis updated this revision to Diff 530671.Jun 12 2023, 2:31 PM
TIFitis marked 6 inline comments as done.

Addressed reviewer comments.

TIFitis updated this revision to Diff 530680.Jun 12 2023, 3:07 PM

Update MLIR BodyGen callback.

TIFitis added inline comments.Jun 12 2023, 3:27 PM
llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
4370

I've changed it.

The emitBlock, emitBranch and emitIfClause functions are ported from Clang btw.
clang/lib/CodeGen/CGStmt.cpp

mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
1357–1362

Clang seems to be doing something similar by calculating typeSize from clang::Type.

In my testing this works for what we support for now which are basic arrays, scalars, etc.

For derived types, array subscripts etc. we would need something more sophisticated. I plan on updating this as we add support.

I think this is now style wise pretty good, I still found some potential problems below.

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

This talks about creating a new block, but reading the function it seems it will just place \p BB. Which one is it?

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
4147

Can we at least assert MapperFunc is non null if standalone is true. It seems like an invariant almost worth documenting, at least with an assertion + message.

4370

That might be so, but given the name and initial documentation, nobody would have expected this function to delete the block, or at least I would not have.

4390

In the call above you pass the AllocaIP here, now it is both times the saveIP. I doubt this is correct. Allocas will end up in the wrong spot, won't they? Do we have a test for that?

TIFitis updated this revision to Diff 531760.Jun 15 2023, 8:11 AM
TIFitis marked 3 inline comments as done.

Addressed reviewer comments

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

Fixed.

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
4147

Added

4390

The CallBacks were ignoring the AllocaIP passed and getting it from the context instead.
I've updated them to use the CallBack argument instead.

This patch https://reviews.llvm.org/D150860 changes Clang to use this function for TargetDataCall emission which tests most aspects of it.

This revision is now accepted and ready to land.Jun 16 2023, 11:03 AM