This is an archive of the discontinued LLVM Phabricator instance.

[Flang][MLIR][OpenMP][OMPIRBuilder] Use target triple to initialize `IsGPU` flag
ClosedPublic

Authored by skatrak on Jun 1 2023, 9:49 AM.

Details

Summary

This patch modifies the construction of the OpenMPIRBuilder to initialize the IsGPU flag using target triple information passed down from the Flang frontend. If not present, it will default to false.

This replicates the behavior currently implemented in Clang, where the CodeGenModule::createOpenMPRuntime() method creates a different CGOpenMPRuntime instance depending on the target triple, which in turn has an effect on the IsGPU flag of the OpenMPIRBuilderConfig object.

Diff Detail

Event Timeline

skatrak created this revision.Jun 1 2023, 9:49 AM
Herald added a project: Restricted Project. · View Herald Transcript
skatrak requested review of this revision.Jun 1 2023, 9:49 AM
mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
1302–1314

Can this logic be moved into the OpenMP IRBuilder?

skatrak added inline comments.Jun 12 2023, 4:09 AM
mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
1302–1314

I think it would be easy to pass in the ArchType enum to the OpenMPIRBuilderConfig object and make this check there. I only did it here because it replicates what it's done in Clang: In CodeGenModule::createOpenMPRuntime() and then the constructors for CGOpenMPRuntime[GPU]. By moving this check into the OpenMPIRBuilderConfig, it would be done twice in the Clang flow (the check cannot really be removed from CodeGenModule::createOpenMPRuntime()), but I'm happy to move the check if that's preferred. Just let me know and I'll work on it, thank you.

mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
1302–1314

I think we should either implement this in Flang's target handling or in the OpenMPIRBuilder.

skatrak updated this revision to Diff 532091.Jun 16 2023, 4:41 AM

Move initialization of IsTargetCodegen flag to the frontend and pass it down to the OpenMPIRBuilder through an MLIR attribute like it is done for IsDevice.

Herald added a project: Restricted Project. · View Herald Transcript
skatrak retitled this revision from [MLIR][OpenMP][OMPIRBuilder] Use target triple to initialize `IsTargetCodegen` flag to [Flang][MLIR][OpenMP][OMPIRBuilder] Use target triple to initialize `IsTargetCodegen` flag.Jun 16 2023, 4:41 AM
skatrak edited the summary of this revision. (Show Details)
skatrak marked an inline comment as done.Jun 16 2023, 4:46 AM
skatrak added inline comments.
mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
1302–1314

Thanks for the suggestion. It seems like it makes more sense then to move it earlier in the flow rather than to the OpenMPIRBuilder at the very end, since that's already what it's done for IsDevice, so I think the logic is easier to follow and it also doesn't conflict with Clang.

skatrak marked an inline comment as done.Jun 28 2023, 7:28 AM

Ping for review! Thank you.

kiranchandramohan accepted this revision.Jul 3 2023, 2:15 AM

Will there be a change in the LLVM IR generated if the option is set? If so, it might be good to have a test.

Have a couple of Nit comments regarding name of the flag. LG.

flang/include/flang/Frontend/LangOptions.def
41–42

Nit: Do you mean only for Device?

flang/tools/bbc/bbc.cpp
138–140

Nit: is this specific for GPU? If so, should we have the name GPU in the name?

This revision is now accepted and ready to land.Jul 3 2023, 2:15 AM
skatrak updated this revision to Diff 538993.Jul 11 2023, 3:11 AM

Rename compiler option and MLIR attribute to match new name for the flag. Create unit test for a case where that flag impacts the generated IR.

Herald added a project: Restricted Project. · View Herald TranscriptJul 11 2023, 3:11 AM
skatrak marked 2 inline comments as done.Jul 11 2023, 3:15 AM

Thank you for your feedback, Kiran.

Will there be a change in the LLVM IR generated if the option is set? If so, it might be good to have a test.

I added an OpenMPIRBuilder test to exercise a code path that is dependent on that flag.

Have a couple of Nit comments regarding name of the flag. LG.

I created D154591 to address the confusion with flag names, because the ones we were using didn't describe very well what they represented. I just updated option / MLIR attribute names here to match what was decided there, which should also address your concerns.

skatrak retitled this revision from [Flang][MLIR][OpenMP][OMPIRBuilder] Use target triple to initialize `IsTargetCodegen` flag to [Flang][MLIR][OpenMP][OMPIRBuilder] Use target triple to initialize `IsGPU` flag.Jul 11 2023, 3:18 AM
skatrak edited the summary of this revision. (Show Details)
skatrak updated this revision to Diff 541054.Jul 17 2023, 8:27 AM

Fix formatting.

Ping! Can this patch be landed now or should I wait for another review?