LG

Add tests for C++ and move OpenMP specific tests to OpenMP directory

In D28907#1120098, @RKSimon wrote:

@dtemirbulatov @ABataev Is there anything that can be done to please keep this patch moving? The next release branch won't be far away now and I originally requested this over a year and a half ago (PR30787), and ideally I'd like to get this in and then I can more easily implement PR33744 as well in time.

I notice there's still some minor refactoring in the patch to use InstructionState in newTreeEntry/tryScheduleBundle calls - is it worth getting that done quickly first?

What about testing - how much testing has been done with external code?

George, could you commit it, please? I need a lot of time to configure openmp runtime so I could commit it.

Address comments from Sanjay Patel

LG

Improved code.

In D45783#1106019, @echristo wrote:

So, I'd really prefer not to set options via the backend option path. From here I think we should aim to take all of the options we added and having the asm printer in the backend know how to set them depending on the target. We could also add things to the IR metadata if necessary, but I'd like to avoid that if possible.

Thoughts?

In D46667#1094340, @Anastasia wrote:

OpenCL C is based on C99, so OpenMP isn't enabled by default. But in your tests you use -fopenmp to activate it.

OpenCL general philosophy is that vectors are written explicitly, but it's not always very easy. In OpenCL C++ we have added an attribute hint for auto vectorization cl::vec_type_hint. It's given to a kernel rather than for a loop though. So I think this simd pragma is useful. My only worry is that other OpenMP features might not work well in OpenCL and we have no way to reject them at the moment.

BTW, the test itself does not check anything.

1. Is this allowed to use OpenMP for OpenCL programs at all?
2. If it is allowed, I think it would be better to set the TypeSourceInfo for the artificial variable

LG

Hmm. Not sure, of course, but looks like the corruption of the output buffer. Like the flush is required or something like this. Need the output result for better analysis.

In D41827#1083570, @echristo wrote:

In D41827#1083441, @echristo wrote:

In D41827#1083429, @ABataev wrote:

In D41827#1083428, @echristo wrote:

In D41827#1083427, @ABataev wrote:

In D41827#1083412, @echristo wrote:

FWIW I'm seeing internal correctness issues after this patch has been merged - I think it's related to the section changing part of the patches. I'm investigating.

:( Very said. Do have troubles with NVPTX target or some other targets?

NVPTX :(

Do you have a repro?

Not yet. Trying to work one up.

So this diff:

• for (const std::string &S : DwarfFiles)
• getStreamer().EmitRawText(S.data()); + for (const std::string &S : DwarfFiles) + getStreamer().EmitRawText(DwarfFiles.front().data());
  
  causes the problem to go away.
  
  The problem shows up this way:
  
  There is more than one initializer with name 'file'
  
  during program startup (within google so you won't be able to duplicate this part). which seems a little weird, but I'm guessing that there's something going on in the way that we're outputting the file changes at section switch time with how ptxas is dealing with it.
  
  For now I'm going to go ahead and revert this because it's making it so that cuda compilation isn't working with clang and I'll continue working up a testcase and see if I can't narrow down the problem a little better and get back to you soon.

In D41827#1083428, @echristo wrote:

In D41827#1083427, @ABataev wrote:

In D41827#1083412, @echristo wrote:

FWIW I'm seeing internal correctness issues after this patch has been merged - I think it's related to the section changing part of the patches. I'm investigating.

:( Very said. Do have troubles with NVPTX target or some other targets?

NVPTX :(

Do you have a repro?

In D41827#1083412, @echristo wrote:

FWIW I'm seeing internal correctness issues after this patch has been merged - I think it's related to the section changing part of the patches. I'm investigating.

:( Very said. Do have troubles with NVPTX target or some other targets?

Rebase against latest version.

Will it work correctly if some of the operations are used several times in the bundles? It would be good to have the tests for this kind of situation.

Hmm, I don't think this is correct. I think it is against the OpenMP standard.
According to OpenMP 4.5:
When an original variable is mapped to a device data environment and the associated 19 corresponding variable is not present in the device data environment, a new corresponding variable 20 (of the same type and size as the original variable) is created in the device data environment.

In D46021#1077023, @dblaikie wrote:

Sorry, I'm a bit confused.

1. I'd expect /this/ behavior (line table directives, but no debug_info, etc) to be controlled via a DICompileUnit emission kind (a value between "NoDebug" and "LineTablesOnly" as Paul mentioned).
2. I'd expect the "no ', debug'" flag (which I don't see in this change? Is it missing? is it elsewhere?) to be controlled via an MCOption - because it must be global.
   
   Now, arguably, if we have the global "no ', debug'" flag we maybe might as well use it to implement this behavior too since we don't have any other use for it? So maybe (2) invalidates (1).

Need to rework the solution after discussion with David Blaike and Paul Robinson.

In D45782#1072437, @tra wrote:

, debug should be removed to enable opt level > 0. I checked the ptxas, without , debug it just ignores DWARF sections, but with -lineinfo option it may generate correct data for the nvprof from .loc and .file directives.

Are you saying that enabling lineinfo debug also produces DWARF sections now and that's why we need the flag to ignore them if optimizations are enabled? Previously there was no need because we were emitting only .loc and .file.