[PATCH] D105981: [AMDGPU][OpenMP] Support linking of math libraries

JonChesterfield added a comment. It's been pointed out to me that -lm is a linker flag so it's weird to require it at compile time. I haven't thought of a good fix for that yet. We don't need to splice in ocml for each compilation unit, so can move the test+splice into the link phase, except th

[PATCH] D105981: [AMDGPU][OpenMP] Support linking of math libraries

This revision was automatically updated to reflect the committed changes. Closed by commit rG9830f902e4d0: [AMDGPU][OpenMP] Support linking of math libraries (authored by pdhaliwal). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105981/new/ https://

[PATCH] D105981: [AMDGPU][OpenMP] Support linking of math libraries

yaxunl accepted this revision. yaxunl added a comment. LGTM. Thanks. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105981/new/ https://reviews.llvm.org/D105981 ___ cfe-commits mailing list cfe-commits@li

[PATCH] D105981: [AMDGPU][OpenMP] Support linking of math libraries

JonChesterfield accepted this revision. JonChesterfield added a comment. This revision is now accepted and ready to land. I think this is good enough for now, assuming the majority of OvO is running locally. More cleanups to do after landing. Repository: rG LLVM Github Monorepo CHANGES SINCE

[PATCH] D105981: [AMDGPU][OpenMP] Support linking of math libraries

pdhaliwal updated this revision to Diff 362718. pdhaliwal added a comment. Missed comment. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105981/new/ https://reviews.llvm.org/D105981 Files: clang/lib/Driver/ToolChains/AMDGPU.cpp clang/lib/Drive

[PATCH] D105981: [AMDGPU][OpenMP] Support linking of math libraries

pdhaliwal updated this revision to Diff 362717. pdhaliwal added a comment. Rename method to getCommonDeviceLibNames Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105981/new/ https://reviews.llvm.org/D105981 Files: clang/lib/Driver/ToolChains/AMD

[PATCH] D105981: [AMDGPU][OpenMP] Support linking of math libraries

yaxunl added inline comments. Comment at: clang/lib/Driver/ToolChains/AMDGPU.cpp:831-860 auto Kind = llvm::AMDGPU::parseArchAMDGCN(GpuArch); const StringRef CanonArch = llvm::AMDGPU::getArchNameAMDGCN(Kind); std::string LibDeviceFile = RocmInstallation.getLibDeviceFile(C

[PATCH] D105981: [AMDGPU][OpenMP] Support linking of math libraries

yaxunl added inline comments. Comment at: clang/lib/Driver/ToolChains/AMDGPU.cpp:831-860 auto Kind = llvm::AMDGPU::parseArchAMDGCN(GpuArch); const StringRef CanonArch = llvm::AMDGPU::getArchNameAMDGCN(Kind); std::string LibDeviceFile = RocmInstallation.getLibDeviceFile(C

[PATCH] D105981: [AMDGPU][OpenMP] Support linking of math libraries

JonChesterfield added inline comments. Comment at: clang/lib/Driver/ToolChains/AMDGPU.cpp:831-860 auto Kind = llvm::AMDGPU::parseArchAMDGCN(GpuArch); const StringRef CanonArch = llvm::AMDGPU::getArchNameAMDGCN(Kind); std::string LibDeviceFile = RocmInstallation.getLibDev

[PATCH] D105981: [AMDGPU][OpenMP] Support linking of math libraries

yaxunl added inline comments. Comment at: clang/lib/Driver/ToolChains/AMDGPU.cpp:831-860 auto Kind = llvm::AMDGPU::parseArchAMDGCN(GpuArch); const StringRef CanonArch = llvm::AMDGPU::getArchNameAMDGCN(Kind); std::string LibDeviceFile = RocmInstallation.getLibDeviceFile(C

[PATCH] D105981: [AMDGPU][OpenMP] Support linking of math libraries

pdhaliwal added a comment. Due to the current state of math headers, I was unable to test this patch without ockl. But last time when headers were working, I was actually required to link ockl for a symbol (I forgot the name). I will update once I am able to get the math headers work again. R

[PATCH] D105981: [AMDGPU][OpenMP] Support linking of math libraries

JonChesterfield added a comment. I don't like that this pulls in ockl automatically but don't think that's a blocker. OK on my side, @yaxunl? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105981/new/ https://reviews.llvm.org/D105981 _

[PATCH] D105981: [AMDGPU][OpenMP] Support linking of math libraries

pdhaliwal added inline comments. Comment at: clang/lib/Driver/ToolChains/AMDGPU.cpp:923-924 + bool CorrectSqrt = DriverArgs.hasFlag( + options::OPT_fhip_fp32_correctly_rounded_divide_sqrt, + options::OPT_fno_hip_fp32_correctly_rounded_divide_sqrt); + bool Wave64 = isW

[PATCH] D105981: [AMDGPU][OpenMP] Support linking of math libraries

pdhaliwal updated this revision to Diff 361967. pdhaliwal added a comment. Extract the options from HIP/OpenMP to a common method in base class. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105981/new/ https://reviews.llvm.org/D105981 Files: cl

[PATCH] D105981: [AMDGPU][OpenMP] Support linking of math libraries

JonChesterfield added inline comments. Comment at: clang/lib/Driver/ToolChains/AMDGPU.cpp:879-889 + // TODO: There are way too many flags that change this. Do we need to check + // them all? + bool DAZ = DriverArgs.hasArg(options::OPT_cl_denorms_are_zero) || + getD

[PATCH] D105981: [AMDGPU][OpenMP] Support linking of math libraries

yaxunl added inline comments. Comment at: clang/lib/Driver/ToolChains/AMDGPU.cpp:879-889 + // TODO: There are way too many flags that change this. Do we need to check + // them all? + bool DAZ = DriverArgs.hasArg(options::OPT_cl_denorms_are_zero) || + getDefaultDen

[PATCH] D105981: [AMDGPU][OpenMP] Support linking of math libraries

JonChesterfield added inline comments. Comment at: clang/lib/Driver/ToolChains/AMDGPU.cpp:879-889 + // TODO: There are way too many flags that change this. Do we need to check + // them all? + bool DAZ = DriverArgs.hasArg(options::OPT_cl_denorms_are_zero) || + getD

[PATCH] D105981: [AMDGPU][OpenMP] Support linking of math libraries

yaxunl added inline comments. Comment at: clang/lib/Driver/ToolChains/AMDGPU.cpp:879-889 + // TODO: There are way too many flags that change this. Do we need to check + // them all? + bool DAZ = DriverArgs.hasArg(options::OPT_cl_denorms_are_zero) || + getDefaultDen

[PATCH] D105981: [AMDGPU][OpenMP] Support linking of math libraries

JonChesterfield added a reviewer: t-tye. JonChesterfield added inline comments. Comment at: clang/lib/Driver/ToolChains/AMDGPUOpenMP.cpp:267 +llvm::SmallVector BCLibs; +BCLibs.append(RocmInstallation.getCommonBitcodeLibs( +DriverArgs, LibDeviceFile, Wave64, DAZ, F

[PATCH] D105981: [AMDGPU][OpenMP] Support linking of math libraries

pdhaliwal updated this revision to Diff 358614. pdhaliwal added a comment. Move linking logic to a common method. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105981/new/ https://reviews.llvm.org/D105981 Files: clang/lib/Driver/ToolChains/AMDGP

[PATCH] D105981: [AMDGPU][OpenMP] Support linking of math libraries

pdhaliwal added inline comments. Comment at: clang/lib/Driver/ToolChains/AMDGPUOpenMP.cpp:252 +bool Wave64 = isWave64(DriverArgs, Kind); + +// TODO: There are way too many flags that change this. Do we need to check JonChesterfield wrote: > I recognise th

[PATCH] D105981: [AMDGPU][OpenMP] Support linking of math libraries

JonChesterfield added inline comments. Comment at: clang/lib/Driver/ToolChains/AMDGPUOpenMP.cpp:252 +bool Wave64 = isWave64(DriverArgs, Kind); + +// TODO: There are way too many flags that change this. Do we need to check I recognise this comment. Is this

[PATCH] D105981: [AMDGPU][OpenMP] Support linking of math libraries

pdhaliwal created this revision. pdhaliwal added reviewers: JonChesterfield, ronlieb, jdoerfert. Herald added subscribers: kerbowa, guansong, t-tye, tpr, dstuttard, yaxunl, nhaehnle, jvesely, kzhuravl. pdhaliwal requested review of this revision. Herald added subscribers: cfe-commits, sstefan1, wd