Added unittest with non-null memory space. Fixed comments to end with '.'.

In D144657#4206620, @ftynse wrote:

Sorry for delay, already-approved patches don't show up in the review stream :(

@sunshaoce, @awarzynski are there remaining issues with this patch? We currently needs this for our testing.

In D144203#4181792, @kiranchandramohan wrote:

Sorry, I haven't had a chance to look at this in detail. I would need to get up to speed with addrspace in LLVM before proceeding with this. I would recommend someone from your organization review this patch and then @jeanPerier sign this off, if it is OK.

@kiranchandramohan did you have any other feedback on this patch?

Fix comment

In D144203#4161422, @kiranchandramohan wrote:

Thanks for the changes and the replies.

Could you also comment on why the address-space cast need not be modelled in FIR?

In D144657#4158826, @krzysz00 wrote:

Looks good to me - though I'm not sure if we want @ftynse or someone else more familiar with DTLI to take a look before landing.

Make types explicit.

Make sure all tests check alloca return type. Use constant value for alloca address space from the ROCDL dialect.

Use Attribute instead of integers to encode memory spaces. Change address space -> memory space.

Also, how would the default case be handled (currently returns 0)?

In D144657#4148055, @krzysz00 wrote:

... in further comments, if we want to go with the address space and memory space as distinct data layout entries system, I'd be willing to review this patch on its own and leave the memory space part to another patch.

In D144203#4137804, @kiranchandramohan wrote:

I am not familiar with address spaces in LLVM. Could you give a pointer to how llvm handles address spaces? Also what is the role of datalayout in address space handling? Currently, it seems that the handling is fully based on the target.

Fixed typo

Add index width test.

In D143102#4103090, @kiranchandramohan wrote:

Would it be possible to add a test for the defaultWidth? Like in flang/test/Fir/target-rewrite-boxchar.fir?

Remove complex arg/return type code.

In D142910#4099238, @agozillon wrote:

In D142910#4097813, @kiranchandramohan wrote:

In D142910#4096441, @agozillon wrote:

From my current work on Declare Target I don't think you necessarily need an omp.function at least so far, you can distinguish what is a device function via other OpenMP operations that are device outlining directives in OpenMP like Declare Target, but the work on Declare Target is very much in the early stages (https://github.com/ROCm-Developer-Tools/llvm-project/pull/185), so we may find some problems along the way. I think the benefits I see of omp.function is perhaps that it may make the code generation process easier, as you can perhaps just directly generate an omp.function from Declare Target clause members that are functions, the global data could be packaged in the declare.target operation, so we effectively skip the middle man. It also makes sure that the device/target lowering that may be necessary is easily separable from the regular FuncOp lowering.

If we require omp.function (may be we should call it omp.target_function) then we would require something like an omp.target_variable as well isn't it?

That is likely true, and maybe a more general approach than having omp.target_declare if there are other ways to create outlineable global variables (which I imagine there is) and just generally a little more flexible for outlined variables. For now I am waiting to see the results of the poll / discussion to see which way everyone leans on this topic!

In D143102#4097773, @tschuett wrote:

I would have expected something ala:

flang -fopenmp -fopenmp-targets=nvptx64-nvidia-cuda example.f90

https://openmp.org/wp-content/uploads/SC18-BoothTalks-Ozen.pdf

gfx902 is not a cpu.

In D143102#4097753, @kiranchandramohan wrote:

It will be helpful if you can share a godbolt link with the types that you see for complex argument and return type like in https://godbolt.org/z/x73xT3r83. I see some difference from what you have given here. Not sure whether it is because of an incorrect invocation.

Please add tests as well (refer to previous commits https://reviews.llvm.org/D136547).

In D142244#4096777, @clementval wrote:

In D142244#4096648, @jsjodin wrote:

In D142244#4096579, @clementval wrote:

In D142244#4096546, @jsjodin wrote:

In D142244#4096538, @clementval wrote:

In D142244#4096512, @jsjodin wrote:

No, it should work for any configuration as far as I know. How are you running the test?

Just with check-flang

How does it fail? Can you show the output? Also what CPU are you running on?

I run on `x86_64`

How did you configure your build? I'd like to reproduce your build and see if I can make it fail on my machine. Right now the test passes.

My bad .. my CompilerInvocation.cpp was overwritten and missed part of this patch. It works fine. Sorry to have bothered you with that.

In D142244#4096579, @clementval wrote:

In D142244#4096546, @jsjodin wrote:

In D142244#4096538, @clementval wrote:

In D142244#4096512, @jsjodin wrote:

No, it should work for any configuration as far as I know. How are you running the test?

Just with check-flang

How does it fail? Can you show the output? Also what CPU are you running on?

I run on `x86_64`

In D142244#4096538, @clementval wrote:

In D142244#4096512, @jsjodin wrote:

No, it should work for any configuration as far as I know. How are you running the test?

Just with check-flang

In D142244#4096492, @clementval wrote:

The new test embed.f90 and embed-error.f90 are failing on my side. Do they assume a specific configuration?

This is no longer needed.

Merge with trunk so patch applies.

In D142244#4081187, @jhuber6 wrote:

Do you know what's left on the Flang driver side to enable offloading?

@awarzynski do you have any other concerns about the patch?

Update variable names to camel case.

Update test to check metadata.

Added test to embed with .bc file as input instead of source file. Fixed comment in negative test.

In D142244#4071213, @awarzynski wrote:

Thanks for sending this, @jsjodin ! Overall, this makes sense. But I have no experience with offloading and I would appreciate some RFC that would explain what the overall direction. What are the next steps? And what's the overall goal?

Added negative test and early return when error occurrs.

This looks good to me. Have @jdoerfert approve as well.

LG

Fix comment and test.

In D138496#3970344, @domada wrote:

@jsjodin thanks for your remark. Let me add some more information to this patch.

Currently clang emits information about preferred simd alignment only for X86, WebAssembly and PPC targets.

[snip]

Your code looks elegant but in my opinion it will generate different code for ARM architecture where the vector alignment is set to 64 ( https://github.com/llvm-mirror/clang/blob/master/lib/Basic/Targets/ARM.cpp#L311 ), but simd default alignment is set to 0.

If we implemented DataLayout::getMaxPreferredVectorTypeAlign something like this:

No need to pass in IsEmbedded, it is in the config now. Perhaps the manager should also have the config available.

Update against trunk.

Make firstSeparator() and separator() return the correct default values when the optional values are not set. This will allow overriding the default, and will avoid inconsistencies in the case IsTargetCodegen is modified in the config. Change name of getName to something more descriptive. Maybe getPlatformSpecificName is better than createPlatformSpecificName, feedback is welcome.

In D138496#3944348, @domada wrote:

Yes, we can extend calculations of the default alignment. My primary goal was to move code from clang into llvm without modification of the code logic.

AFAIK the default simd alignment corresponds to the maximal width of the simd instruction for given target.

Use the new config class. Perhaps the separators could be inferred from isTargetCodgen, but keeping them explicit for now.

Looking at the existing code before the patch:
  / Return default simd alignment for the given target. Generally, this
  / value is type-specific, but this alignment can be used for most of the
  /// types for the given target.
  unsigned getSimdDefaultAlign() const { return SimdDefaultAlign; }
It is very imprecise. Which types? The smallest alignment for all vector and element types?.

Fix typo in comment.

In D138220#3934395, @tschuett wrote:

Optional<bool> is an odd data structure. I would use enums instead to describe the different states.

In D137725#3930994, @jdoerfert wrote:

In D137725#3930985, @jsjodin wrote:

I'm currently working on using llvm::Optional for the various attiibutes, since that is more flexible. Let me know if you have any other ideas how to handle this.

Optional is good, for the fields or the config object. Alternatively defaults as we can derive them would be OK too. A mix might be good, if we can derive a sensible default, we use it, if not, it's Optional and None.

In D137725#3921567, @jsjodin wrote:

In D137725#3919652, @jdoerfert wrote:

Maybe we want a configuration object that is populated once by the user and passed to the OMPIRBuilder to cluster all such configurations, e.g., device, embedded, etc. We can also choose separators based on that internally.

Yes, I think that sounds good. Formalizing the attributes that affect codgen (in a configuration object) should make the code easier to understand/maintain. I can start working on a patch to add the configuration object.

In D137725#3919652, @jdoerfert wrote:

Maybe we want a configuration object that is populated once by the user and passed to the OMPIRBuilder to cluster all such configurations, e.g., device, embedded, etc. We can also choose separators based on that internally.

Changed setOutlinedFunctionAttributes to setOutlinedTargetRegionFunctionAttributes. Fixed else after return. Added TODO. Removed comment.

In D134816#3879344, @jsjodin wrote:

In D134816#3871176, @jsjodin wrote:

In D134816#3867101, @jsjodin wrote:

In D134816#3866417, @mikerice wrote:

In D134816#3860950, @jsjodin wrote:

> I think it makes sense to do your cleanup first, then migrate to OMPIRbuilder, and do this feature improvement last. That way we have parity first then extend the functionality.

Makes sense to me too. @jsjodin are you going to add the cleanup or should I do it? After that I can fix this patch up.

I am working on the cleanup. Doing some debugging, but the code is mostly there.

I was being a bit too ambitious trying to combine the cleanup and the migration. I will split things up to simplify the changes.

Here is the cleanup patch: https://reviews.llvm.org/D136601
Note that it changes the naming for variable because it was inconsistent with target regions "omp_offloading__..." vs "omp_offloading_..." so 1 underscore instead of 2 at the end of the prefix.

Use std::function instead of function_ref.