[clang] Invert condition refactored in #160935

[jansvoboda11]

The PR #160935 incorrectly replaced llvm::sys::fs::getUniqueID() with llvm::vfs::FileSystem::exists() in a condition. That's incorrect, since the first function returns std::error_code that evaluates to true when there is an error (file doesn't exist), while the new code does the opposite. This PR fixes that issue by inverting the conditional.

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-codegen

Author: Jan Svoboda (jansvoboda11)

Changes

The PR #160935 incorrectly replaced llvm::sys::fs::getUniqueID() with llvm::vfs::FileSystem::exists() in a condition. That's incorrect, since the first function returns std::error_code that evaluates to true when there is an error (file doesn't exist), while the new code does the opposite. This PR fixes that issue by inverting the conditional.

---

Full diff: https://github.com/llvm/llvm-project/pull/161583.diff

2 Files Affected:

• (modified) clang/lib/CodeGen/CGOpenMPRuntime.cpp (+1-1)
• (modified) clang/test/OpenMP/amdgcn_save_temps.c (-2)

diff --git a/clang/lib/CodeGen/CGOpenMPRuntime.cpp b/clang/lib/CodeGen/CGOpenMPRuntime.cpp
index 75bde3f72c4c2..8cda583313ca4 100644
--- a/clang/lib/CodeGen/CGOpenMPRuntime.cpp
+++ b/clang/lib/CodeGen/CGOpenMPRuntime.cpp
@@ -1542,7 +1542,7 @@ static llvm::TargetRegionEntryInfo getEntryInfoFromPresumedLoc(
     SourceManager &SM = CGM.getContext().getSourceManager();
     PresumedLoc PLoc = SM.getPresumedLoc(BeginLoc);
 
-    if (CGM.getFileSystem()->exists(PLoc.getFilename()))
+    if (!CGM.getFileSystem()->exists(PLoc.getFilename()))
       PLoc = SM.getPresumedLoc(BeginLoc, /*UseLineDirectives=*/false);
 
     return std::pair<std::string, uint64_t>(PLoc.getFilename(), PLoc.getLine());
diff --git a/clang/test/OpenMP/amdgcn_save_temps.c b/clang/test/OpenMP/amdgcn_save_temps.c
index ebf0d6031ee82..d838bb1166b6b 100644
--- a/clang/test/OpenMP/amdgcn_save_temps.c
+++ b/clang/test/OpenMP/amdgcn_save_temps.c
@@ -1,8 +1,6 @@
 
 // REQUIRES: amdgpu-registered-target
 
-// XFAIL: *
-
 // RUN: %clang_cc1 -E -fopenmp -x c -triple amdgcn-amd-amdhsa -fopenmp-targets=amdgcn-amd-amdhsa -save-temps=cwd %s -o %t-openmp-amdgcn-amd-amdhsa-gfx90a.i
 // RUN: %clang_cc1 -fopenmp  -x c -triple x86_64-unknown-unknown -fopenmp-targets=amdgcn-amd-amdhsa -save-temps=cwd -emit-llvm-bc %s -o %t-x86_64-unknown-unknown.bc
 // RUN: %clang_cc1 -fopenmp -x c -triple amdgcn-amd-amdhsa -fopenmp-targets=amdgcn-amd-amdhsa -save-temps=cwd -emit-llvm -fopenmp-is-target-device -x cpp-output %t-openmp-amdgcn-amd-amdhsa-gfx90a.i -fopenmp-host-ir-file-path %t-x86_64-unknown-unknown.bc -o - | FileCheck %s

[carlobertolli]

LGTM, thanks for the quick response.

[carlobertolli]

One thing I would like to see is an example where the then statement is actually executed, i.e. what is that PLoc = .... false for?
Not urgent, merging this pr is more important.

[jansvoboda11]

Looking at the code, this condition should kick in when BeginLoc is preceded by a #line that points to a file that doesn't exist - in that case we ask for the source location again, now ignoring that directive (i.e. resolving to the file the BeginLoc physically appears in). I agree more test coverage would be nice here, but I don't have more time to invest into chasing that.