[SYCL] Fix compiler crash. #12324

zahiraam · 2024-01-08T21:52:39Z

The compiler was crashing when the user requested fp-accuracy for the functions in a call of the form f1(f2(f3 ...), where f1, f2 and f3 were fpbuiltin but the innermost function didn't have an fpbuiltin. The current builtinID was used instead of getting the builtinID from the current function. that created a crash in the compiler.
This patch fixes the issue and renames the function EmitFPBuiltinIndirectCall to MaybeEmitFPBuiltinofFD .

github-actions · 2024-01-08T22:40:09Z

✅ With the latest revision this PR passed the C/C++ code formatter.

andykaylor · 2024-01-09T18:06:30Z

clang/lib/CodeGen/CGBuiltin.cpp

@@ -22872,6 +22872,10 @@ llvm::CallInst *CodeGenFunction::EmitFPBuiltinIndirectCall(
    // only if it has an fpbuiltin intrinsic.
    unsigned BuiltinID = getCurrentBuiltinID();
    Name = CGM.getContext().BuiltinInfo.getName(BuiltinID);
+    if (!FD->getNameInfo().getName().isIdentifier() ||


As I mentioned in Teams, this doesn't look like the right fix to me. I think the problem is that we somehow got here when the call wasn't an indirect call. I think it would be better to check that in EmitCall before EmitFPBuiltinIndirectCall is called.

andykaylor · 2024-01-17T19:16:11Z

clang/lib/CodeGen/CGCall.cpp

@@ -5692,7 +5692,7 @@ RValue CodeGenFunction::EmitCall(const CGFunctionInfo &CallInfo,
        !getLangOpts().FPAccuracyVal.empty()) {
      const auto *FD = dyn_cast_if_present<FunctionDecl>(TargetDecl);
      if (FD) {
-        CI = EmitFPBuiltinIndirectCall(IRFuncTy, IRCallArgs, CalleePtr, FD);
+        CI = EmitFPBuiltinofFD(IRFuncTy, IRCallArgs, CalleePtr, FD);


Does it make sense to add a check here to see if this is a function that needs fpbuiltin support?

I am checking this in the function EmitFPBuiltinofFD. If the function FD has not BuiltinID but is one of the function listed in EmitFPBuiltinofFD (~ line# 22861), an attribute is generated for it. Is that what you mean, or am I missing something?

I just thought that if you checked it here you wouldn't need to call EmitPDBuiltinofFD at all in many cases.

aelovikov-intel · 2024-01-19T21:20:54Z

sycl/test/basic_tests/fp-accuracy.cpp

+// RUN: %clangxx -%fsycl-host-only -c -ffp-accuracy=high \
+// RUN: -faltmathlib=SVMLAltMathLibrary -fno-math-errno %s
+
+// RUN: %clangxx -fsycl %s -isystem %sycl_include/sycl -isystem %sycl_include \


Why do we need extra -isystem here?

aelovikov-intel · 2024-01-19T21:22:15Z

sycl/test/basic_tests/fp-accuracy.cpp

+// CHECK-LABEL: define {{.*}}spir_kernel void @{{.*}}Kernel2
+// CHECK: tail call double @llvm.fpbuiltin.log.f64(double %2) #[[ATTR_HIGH:[0-9]+]]
+// CHECK: tail call double @llvm.fpbuiltin.exp.f64(double %3) #[[ATTR_HIGH]]
+// CHECK: tail call double @llvm.fpbuiltin.cos.f64(double %4) #[[ATTR_HIGH]]


Are you trying to verify device or host LLVM IR? If the former, then it should go into https://github.com/intel/llvm/tree/sycl/sycl/test/check_device_code.

Moved the test to test/check_device_code.

andykaylor · 2024-01-19T21:24:41Z

clang/lib/CodeGen/CGCall.cpp

@@ -5031,6 +5031,23 @@ static unsigned getMaxVectorWidth(const llvm::Type *Ty) {
  return MaxVectorWidth;
 }

+static bool shouldCreateFPBuiltinForFD(const FunctionDecl *FD, StringRef Name) {


This is close to what I was thinking, but if getBuiltinID() returns a non-zero value, it still might be a non-fp-related builtin, and if we have an accuracy map, the name might not be in the map. So we could still be calling EmitFPBuiltinofFD() for a lot of functions that don't need it.

On the other hand, I see from even just this limited implementation that there's necessarily going to be duplication of the list of handled functions and builtins between this check and the EmitFPBuiltinofFD() implementation, so maybe it would be better to rename it something like MaybeEmitFPBuiltinofFD() and skip the check I had requested.

This seems like a lot of work for the calls that don't need this, but I don't have an idea to avoid it.

Yes, that's the dilemma! There will be duplication. I will remove the check and rename the function unless I come up with something else. But at this point I have looked at it from every corner!

by reviewer.

aelovikov-intel · 2024-01-22T20:59:16Z

sycl/test/check_device_code/fp-accuracy.cpp

@@ -0,0 +1,48 @@
+// RUN: %clangxx -fsycl %s -isystem %sycl_include/sycl \


isystem is still here. Is that intentional? If so, why?

Sorry. Removed one and not the other. Done now.

aelovikov-intel · 2024-01-22T22:50:22Z

sycl/test/check_device_code/fp-accuracy.cpp

+  deviceQueue.submit([&](handler &cgh) {
+    accessor in_vals{in, cgh, read_only};
+    accessor out_vals{out, cgh, write_only};
+    cgh.single_task<class Kernel1>([=]() {


I don't think you need main/queue/submit here at all. Just use

#include <sycl/sycl.hpp> SYCL_EXTERNAL auto foo(double x) { using namespace sycl; return cos(exp(log(x))); }

aelovikov-intel · 2024-01-22T22:51:11Z

sycl/test/check_device_code/fp-accuracy.cpp

@@ -0,0 +1,47 @@
+// RUN: %clangxx -fsycl %s -ffp-accuracy=high -fno-math-errno \


You also need -fsycl-device-only to avoid host compilation. I'd also recommend to structure RUN lines somewhat like this:

// RUN: %clangxx -fsycl -fsycl-device-only %s -fno-math-errno -ffp-accuracy-high \ // RUN: -S -emit-llvm -o - | FileCheck %s // RUN: %clangxx -fsycl -fsycl-device-only %s -fno-math-errno -ffp-accuracy-high \ // RUN: -ffp-accuracy=low:exp \ // RUN: -S -emit-llvm -o - | FileCheck %s --check-prefix=CHECK-LOW-EXP

to ensure that common options appear the same and differences are easy to spot.

Maybe even use:

// DEFINE: %{common_opts} = -fsycl -fsycl-device-only -fno-math-errno -ffp-accuracy-high -S -emit-llvm -o - // RUN: %clangxx %{common_opts} | FileCheck %s // RUN: %clangxx %{common_opts} -ffp-accuracy=low:exp | FileCheck %s --check-prefix=CHECK-LOW-EXP

@aelovikov-intel Thanks. Modified per your 2nd suggestion.

aelovikov-intel

LGTM for the test under sycl/test

zahiraam · 2024-01-24T15:45:37Z

@intel/dpcpp-cfe-reviewers can you please take a look? Thanks.

elizabethandrews

I'm not familiar with floating point semantics and so am not really sure how to review the functionality. @premanandrao @andykaylor can you take a look?

premanandrao · 2024-01-25T20:54:11Z

I'm not familiar with floating point semantics and so am not really sure how to review the functionality. @premanandrao @andykaylor can you take a look?

I am okay with the FE changes.

Thanks for the ping @elizabethandrews; I thought I had approved it yesterday, but guess I didn't.

zahiraam · 2024-02-01T14:13:56Z

@intel/llvm-gatekeepers Can this be merged in please? Thanks.

ldrumm · 2024-02-01T14:25:05Z

@intel/llvm-gatekeepers Can this be merged in please? Thanks.

This isn't ready. There are several questions unresolved. Please check with @andykaylor for resolution. Then I'd be happy to merge

zahiraam · 2024-02-01T14:32:31Z

Elizabeth has asked Prem to review who has given his approval. @elizabethandrews can you please review? Thanks.
Andy asked to rename the function (which was done) after many discussions offline. @andykaylor can you please review?

ldrumm · 2024-02-01T14:38:23Z

Sorry. Managed to edit your comment rather than responding.
What I meant to say as a response, rather than an edit

There's also this big comment:

As I mentioned in Teams, this doesn't look like the right fix to me. I think the problem is that we somehow got here when the call wasn't an indirect call. I think it would be better to check that in EmitCall before EmitFPBuiltinIndirectCall is called.

That's a valid and serious question about the fundamental nature of the fix. I see you've fixed up many other comments, but the above needs to be corrected.

zahiraam · 2024-02-01T15:02:08Z

Sorry. Managed to edit your comment rather than responding. What I meant to say as a response, rather than an edit

There's also this big comment:
As I mentioned in Teams, this doesn't look like the right fix to me. I think the problem is that we somehow got here when the call wasn't an indirect call. I think it would be better to check that in EmitCall before EmitFPBuiltinIndirectCall is called.
That's a valid and serious question about the fundamental nature of the fix. I see you've fixed up many other comments, but the above needs to be corrected.

I looked though the history of our conversation with @andykaylor. The reason we can't do what Andy is proposing is that we would have to create additional llvm::builtin for some of the math functions such as llvm.acos and that would diverge from community. At any case, let's see what Andy has to say about it. Thanks.

ldrumm · 2024-02-01T15:07:54Z

If that's the case it may be better to fix this upstream?

zahiraam · 2024-02-01T15:10:00Z

If that's the case it may be better to fix this upstream?

This is not an option that's used upstream.

andykaylor · 2024-02-01T16:45:43Z

Sorry. Managed to edit your comment rather than responding. What I meant to say as a response, rather than an edit
There's also this big comment:
As I mentioned in Teams, this doesn't look like the right fix to me. I think the problem is that we somehow got here when the call wasn't an indirect call. I think it would be better to check that in EmitCall before EmitFPBuiltinIndirectCall is called.
That's a valid and serious question about the fundamental nature of the fix. I see you've fixed up many other comments, but the above needs to be corrected.
I looked though the history of our conversation with @andykaylor. The reason we can't do what Andy is proposing is that we would have to create additional llvm::builtin for some of the math functions such as llvm.acos and that would diverge from community. At any case, let's see what Andy has to say about it. Thanks.

Yes, I should have marked that as resolved. Zahira and I have talked about this, and I'm satisfied with the current implementation.

ldrumm · 2024-02-01T17:07:57Z

Thanks for the clarifications @andykaylor @zahiraam

merged

Fix compiler crash.

9c99d16

zahiraam temporarily deployed to WindowsCILock January 8, 2024 21:53 — with GitHub Actions Inactive

zahiraam requested a review from andykaylor January 8, 2024 21:53

zahiraam temporarily deployed to WindowsCILock January 8, 2024 22:24 — with GitHub Actions Inactive

andykaylor reviewed Jan 9, 2024

View reviewed changes

zahiraam added 3 commits January 14, 2024 12:13

Proposing another solution to fix the crash.

c134c72

Fix format.

9a3487f

Merge remote-tracking branch 'origin/sycl' into FixAssertion

6d56418

zahiraam temporarily deployed to WindowsCILock January 15, 2024 13:15 — with GitHub Actions Inactive

zahiraam temporarily deployed to WindowsCILock January 15, 2024 13:47 — with GitHub Actions Inactive

zahiraam requested a review from andykaylor January 16, 2024 22:39

andykaylor reviewed Jan 17, 2024

View reviewed changes

Added a check to see if FD needs fpbuiltin support.

c7faef2

zahiraam requested a review from andykaylor January 19, 2024 21:03

zahiraam temporarily deployed to WindowsCILock January 19, 2024 21:04 — with GitHub Actions Inactive

zahiraam marked this pull request as ready for review January 19, 2024 21:04

zahiraam requested review from a team as code owners January 19, 2024 21:04

zahiraam requested a review from aelovikov-intel January 19, 2024 21:04

aelovikov-intel reviewed Jan 19, 2024

View reviewed changes

andykaylor reviewed Jan 19, 2024

View reviewed changes

zahiraam had a problem deploying to WindowsCILock January 19, 2024 21:40 — with GitHub Actions Failure

Fixed LIT tests and replace the name of the function as suggested

8ae21fe

by reviewer.

zahiraam had a problem deploying to WindowsCILock January 22, 2024 19:16 — with GitHub Actions Failure

zahiraam temporarily deployed to WindowsCILock January 22, 2024 19:59 — with GitHub Actions Inactive

zahiraam requested review from andykaylor and aelovikov-intel January 22, 2024 20:44

aelovikov-intel reviewed Jan 22, 2024

View reviewed changes

Fixed LIT test again.

25943e5

zahiraam temporarily deployed to WindowsCILock January 22, 2024 22:43 — with GitHub Actions Inactive

aelovikov-intel reviewed Jan 22, 2024

View reviewed changes

zahiraam temporarily deployed to WindowsCILock January 22, 2024 23:15 — with GitHub Actions Inactive

Edited LIT test as suggested by reviewer.

c53de1d

zahiraam requested a review from aelovikov-intel January 23, 2024 15:32

zahiraam temporarily deployed to WindowsCILock January 23, 2024 15:40 — with GitHub Actions Inactive

zahiraam temporarily deployed to WindowsCILock January 23, 2024 16:12 — with GitHub Actions Inactive

aelovikov-intel approved these changes Jan 23, 2024

View reviewed changes

elizabethandrews reviewed Jan 24, 2024

View reviewed changes

premanandrao approved these changes Jan 25, 2024

View reviewed changes

ldrumm merged commit 4fdcb58 into intel:sycl Feb 1, 2024

zahiraam deleted the FixAssertion branch August 12, 2024 20:58

		@@ -0,0 +1,48 @@
		// RUN: %clangxx -fsycl %s -isystem %sycl_include/sycl \

		@@ -0,0 +1,47 @@
		// RUN: %clangxx -fsycl %s -ffp-accuracy=high -fno-math-errno \

[SYCL] Fix compiler crash. #12324

[SYCL] Fix compiler crash. #12324

Uh oh!

Conversation

zahiraam commented Jan 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jan 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aelovikov-intel left a comment

Choose a reason for hiding this comment

Uh oh!

zahiraam commented Jan 24, 2024

Uh oh!

elizabethandrews left a comment

Choose a reason for hiding this comment

Uh oh!

premanandrao commented Jan 25, 2024

Uh oh!

zahiraam commented Feb 1, 2024

Uh oh!

ldrumm commented Feb 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zahiraam commented Feb 1, 2024 • edited by ldrumm Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ldrumm commented Feb 1, 2024

Uh oh!

zahiraam commented Feb 1, 2024

Uh oh!

ldrumm commented Feb 1, 2024

Uh oh!

zahiraam commented Feb 1, 2024

Uh oh!

andykaylor commented Feb 1, 2024

Uh oh!

ldrumm commented Feb 1, 2024

Uh oh!

Uh oh!

zahiraam commented Jan 8, 2024 •

edited

Loading

github-actions bot commented Jan 8, 2024 •

edited

Loading

ldrumm commented Feb 1, 2024 •

edited

Loading

zahiraam commented Feb 1, 2024 •

edited by ldrumm

Loading