yaxunl added inline comments.
================ Comment at: clang/lib/CodeGen/TargetInfo.cpp:9436 + CGM.getModule(), Type, true, + llvm::GlobalValue::LinkageTypes::LinkOnceODRLinkage, + llvm::ConstantInt::get(Type, Value), Name, nullptr, ---------------- jhuber6 wrote: > yaxunl wrote: > > jhuber6 wrote: > > > yaxunl wrote: > > > > yaxunl wrote: > > > > > jhuber6 wrote: > > > > > > yaxunl wrote: > > > > > > > This does not support per-TU control variables. Probably should > > > > > > > use internal linkage. > > > > > > The AMDGPU device libraries use `linkone_odr` so I figured it was > > > > > > the most appropriate here. It should mean that we can have multiple > > > > > > identical definitions and they don't clash. There's also no > > > > > > requirement for these to be emitted as symbols AFAIK. > > > > > > The AMDGPU device libraries use `linkone_odr` so I figured it was > > > > > > the most appropriate here. It should mean that we can have multiple > > > > > > identical definitions and they don't clash. There's also no > > > > > > requirement for these to be emitted as symbols AFAIK. > > > > > > > > > > clang uses -mlink-builtin-bitcode to link these device libraries for > > > > > HIP and OpenCL. Then the linkage of these variables becomes internal > > > > > linkage. That's why it works for per-TU control. > > > > > > The AMDGPU device libraries use `linkone_odr` so I figured it was > > > > > > the most appropriate here. It should mean that we can have multiple > > > > > > identical definitions and they don't clash. There's also no > > > > > > requirement for these to be emitted as symbols AFAIK. > > > > > > > > > > clang uses -mlink-builtin-bitcode to link these device libraries for > > > > > HIP and OpenCL. Then the linkage of these variables becomes internal > > > > > linkage. That's why it works for per-TU control. > > > > > > > > You may let HIP and OpenCL use internal linkage and C/C++/OpenMP use > > > > linkonce_odr since only HIP and OpenCL toolchain use > > > > -mlink-builtin-bitcode to link these device libraries > > > I see, `linkonce_odr` implies that these should all have the same value > > > which isn't necessarily true after linking. I'll change it to use private > > > linkage. > > > > > > OpenMP right now links everything late which means that we don't allow > > > these to be defined differently per-TU. This may be incorrect given this > > > new method as each TU will have different things set. I can change OpenMP > > > to use the `mlink` method after this patch which may be more strictly > > > correct. > > > I see, `linkonce_odr` implies that these should all have the same value > > > which isn't necessarily true after linking. I'll change it to use private > > > linkage. > > > > > > OpenMP right now links everything late which means that we don't allow > > > these to be defined differently per-TU. This may be incorrect given this > > > new method as each TU will have different things set. I can change OpenMP > > > to use the `mlink` method after this patch which may be more strictly > > > correct. > > > > On second thoughts, the idea for letting clang to emit these control > > variables might not work for HIP and OpenCL. The reason is that to support > > per-TU control variables, these variables need to be internal or private > > linkage, however, that means they cannot be used by other device library > > functions which are expecting non-internal linkage for them. Those device > > library functions will end up using control variables from device library > > bitcode any way. > > > > For OpenMP, it may be necessary to emit them as linkonce_odr, otherwise > > device library functions may not find them. > > On second thoughts, the idea for letting clang to emit these control > > variables might not work for HIP and OpenCL. The reason is that to support > > per-TU control variables, these variables need to be internal or private > > linkage, however, that means they cannot be used by other device library > > functions which are expecting non-internal linkage for them. Those device > > library functions will end up using control variables from device library > > bitcode any way. > > Right now we include each file per-TU using `-mlink-builtin-bitcode` which > converts `linkonce_odr` to `private` linkage. Shouldn't this be equivalent? > It may be possible to make some test showing a user of these constants to > verify they get picked up correctly. If you're worried about these getting > removed we may be able to stash them in `compiler.used`, that shouldn't > impede the necessary constant propagation. > > Side note, OpenCL seems to optimize these out without `-disable-llvm-optzns` > while HIP will not. Does OpenCL use some mandatory passes to ensure that > these control variables get handled? This method of using control constants > in general is somewhat problematic as it hides invalid code behind some > mandatory CP and DCE passes. For OpenMP right now we just generate one > version for each architecture, which is wasteful but somewhat easier to work > with. > > > On second thoughts, the idea for letting clang to emit these control > > variables might not work for HIP and OpenCL. The reason is that to support > > per-TU control variables, these variables need to be internal or private > > linkage, however, that means they cannot be used by other device library > > functions which are expecting non-internal linkage for them. Those device > > library functions will end up using control variables from device library > > bitcode any way. > > Right now we include each file per-TU using `-mlink-builtin-bitcode` which > converts `linkonce_odr` to `private` linkage. Shouldn't this be equivalent? > It may be possible to make some test showing a user of these constants to > verify they get picked up correctly. If you're worried about these getting > removed we may be able to stash them in `compiler.used`, that shouldn't > impede the necessary constant propagation. > Let's assume the main program calls `foo()` and `foo()` uses a control variable `bar`. `foo()` is in a bitcode linked in by -mlink-builtin-bitcode. clang emits the control variable `bar` with private linkage in the main module. When clang tries to link `foo()`, it needs to resolve `bar`, but it cannot use the `bar` in the main module because `bar` has private linkage. Then `bar` becomes unresolved. > Side note, OpenCL seems to optimize these out without `-disable-llvm-optzns` > while HIP will not. Does OpenCL use some mandatory passes to ensure that > these control variables get handled? This method of using control constants > in general is somewhat problematic as it hides invalid code behind some > mandatory CP and DCE passes. For OpenMP right now we just generate one > version for each architecture, which is wasteful but somewhat easier to work > with. > Are you using clang -cc1 without other options? There are LLVM passes by default, but they should not depend on language. You can see which pass is removing them by -mllvm -print-after-all. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130096/new/ https://reviews.llvm.org/D130096 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
