llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
27.84k stars 11.47k forks source link

[OpenMP] Static class members not implicitly captured from derived classes #93869

Open jhuber6 opened 3 months ago

jhuber6 commented 3 months ago

When a derived class is present in a target region it will try to automatically declare members on the device. This is not correctly handled when done through a derived class that references the parent. See the example on godbolt https://godbolt.org/z/zvG5nr6KW and below.

class C {
public:
  C() { asm volatile(""); }
  virtual ~C() { asm volatile(""); }
  static int x;
};

int C::x = 0;
class D : public C {
public:
  D() { x = 1; }
};

void foo() {
#pragma omp target
  {
    D d;
  }
}

Here, the variable x is not captured on the device and thus shows up as an extern variable. This is then never resolved and fails at link time.

jhuber6 commented 3 months ago

@alexey-bataev Any clue where this logic lives?

alexey-bataev commented 3 months ago

The variable C::x is just declared, not defined, see https://godbolt.org/z/Mo3aK578h for example. So, it is correct that it is just marked extern.

jhuber6 commented 3 months ago

The variable C::x is just declared, not defined, see https://godbolt.org/z/Mo3aK578h for example. So, it is correct that it is just marked extern.

Whoops, forgot to copy that bit when I reduced this. I've updated the issue. If you explicitly declare it for the device it works, see https://godbolt.org/z/bPMx3h3o8. My guess is that it doesn't automatically add variables outside of the class definition.

jhuber6 commented 3 months ago

Turns out you don't even need the inheritance https://godbolt.org/z/3ErrG1YPf.

alexey-bataev commented 3 months ago

You need explicitly mark definition as declare target

jhuber6 commented 3 months ago

You need explicitly mark definition as declare target

Okay, so this is a wontfix? I don't know the expected OpenMP standards behavior for this.

alexey-bataev commented 3 months ago

Need to check the standard, previously it was a feature, not a bug :)

jhuber6 commented 3 months ago

Need to check the standard, previously it was a feature, not a bug :)

Alright, this is related to a V&V failure I reduced https://github.com/OpenMP-Validation-and-Verification/OpenMP_VV/blob/master/tests/5.2/unified_shared_mem/test_target_VirDestr.cpp.

llvmbot commented 3 months ago

@llvm/issue-subscribers-openmp

Author: Joseph Huber (jhuber6)

When a derived class is present in a target region it will try to automatically declare members on the device. This is not correctly handled when done through a derived class that references the parent. See the example on godbolt https://godbolt.org/z/zvG5nr6KW and below. ```c++ class C { public: C() { asm volatile(""); } virtual ~C() { asm volatile(""); } static int x; }; int C::x = 0; class D : public C { public: D() { x = 1; } }; void foo() { #pragma omp target { D d; } } ``` Here, the variable `x` is not captured on the device and thus shows up as an `extern` variable. This is then never resolved and fails at link time.
alexey-bataev commented 3 months ago

This is for unified shared memory, I suppose definition on the host must be visible from the device in this case.