[SYCL][FPGA] Update __builtin_intel_fpga_mem #5002

Fznamznon · 2021-11-22T08:22:25Z

According to #4980 this built-in
should take four more constant integer parameters with following default
values:

const int32_t AnchorID = -1
const int32_t TargetAnchor = 0
const int32_t Type = 0
const int32_t Cycle = 0

The old variant is still allowed for backward compatibility. In case
some parameters are not provided - default value will be emitted in
annotation string.

According to intel#4980 this built-in should take four more constant integer parameters with following default values: ``` const int32_t AnchorID = -1 const int32_t TargetAnchor = 0 const int32_t Type = 0 const int32_t Cycle = 0 ``` The old variant is still allowed for backward compatibility. In case some parameters are not provided - default value will be emitted in annotation string.

Fznamznon · 2021-11-22T08:25:01Z

@shuoniu-intel , please take a look to make sure we have the same understanding of how this is supposed to work. Please also let me know if strings emitted in LLVM IR i.e. anchor-id, target-anchor, type, cycle are ok.

shuoniu-intel · 2021-11-22T14:55:19Z

@shuoniu-intel , please take a look to make sure we have the same understanding of how this is supposed to work. Please also let me know if strings emitted in LLVM IR i.e. anchor-id, target-anchor, type, cycle are ok.

LGTM!

Fznamznon · 2021-11-29T14:42:35Z

Ping @elizabethandrews , @premanandrao , @smanna12 on review.

premanandrao

Changes look good to me. I just have minor nits.

clang/lib/CodeGen/CGBuiltin.cpp

clang/lib/Sema/SemaChecking.cpp

clang/test/CodeGenSYCL/intel-fpga-mem-builtin.cpp

clang/test/SemaSYCL/intel-fpga-mem-builtin.cpp

smanna12

LGTM

premanandrao

LGTM

Fznamznon · 2021-11-30T21:37:34Z

/summary:run

Fznamznon · 2021-12-02T07:45:23Z

@shuoniu-intel , Is it safe to merge this one? Does FPGA compiler support new form of the annotations? Or maybe we should wait for the spec?

bader · 2021-12-02T10:14:23Z

Is it safe to merge this one?

I think the spec must be approved and merged before the implementation.

mlychkov · 2021-12-02T10:44:20Z

clang/lib/CodeGen/CGBuiltin.cpp

+  auto AddOptionalArgValue = [&E, &Ctx, &Out](int DefaultValue,
+                                              unsigned NumOfArg,
+                                              StringRef StringToAdd) {
+    Optional<llvm::APSInt> IntVal =
+        (E->getNumArgs() > NumOfArg)
+            ? E->getArg(NumOfArg)->getIntegerConstantExpr(Ctx)
+            : APSInt::get(DefaultValue);
+    assert(IntVal.hasValue() && "Constant arg isn't actually constant?");
+    Out << "{" << StringToAdd << ":" << toString(*IntVal, 10) << "}";
+  };
+
+  AddOptionalArgValue(-1, 3, "anchor-id");
+  AddOptionalArgValue(0, 4, "target-anchor");
+  AddOptionalArgValue(0, 5, "type");
+  AddOptionalArgValue(0, 6, "cycle");


May we use AddOptionalArgValue for first two parameters as well (maybe to rename lambda to AddArgValue)?

Suggested change

auto AddOptionalArgValue = [&E, &Ctx, &Out](int DefaultValue,

unsigned NumOfArg,

StringRef StringToAdd) {

Optional<llvm::APSInt> IntVal =

(E->getNumArgs() > NumOfArg)

? E->getArg(NumOfArg)->getIntegerConstantExpr(Ctx)

: APSInt::get(DefaultValue);

assert(IntVal.hasValue() && "Constant arg isn't actually constant?");

Out << "{" << StringToAdd << ":" << toString(*IntVal, 10) << "}";

};

AddOptionalArgValue(-1, 3, "anchor-id");

AddOptionalArgValue(0, 4, "target-anchor");

AddOptionalArgValue(0, 5, "type");

AddOptionalArgValue(0, 6, "cycle");

auto AddArgValue = [&E, &Ctx, &Out](unsigned NumOfArg,

StringRef StringToAdd,

int DefaultValue = MIN_INT) {

Optional<llvm::APSInt> IntVal =

(E->getNumArgs() > NumOfArg)

? E->getArg(NumOfArg)->getIntegerConstantExpr(Ctx)

: APSInt::get(DefaultValue);

assert(IntVal.hasValue() && "Constant arg isn't actually constant?");

Out << "{" << StringToAdd << ":" << toString(*IntVal, 10) << "}";

};

AddArgValue(1, "params");

AddArgValue(2, "cache-size");

AddArgValue(3, "anchor-id", -1);

AddArgValue(4, "target-anchor", 0);

AddArgValue(5, "type", 0);

AddArgValue(6, "cycle", 0);

shuoniu-intel · 2021-12-02T14:07:31Z

@shuoniu-intel , Is it safe to merge this one? Does FPGA compiler support new form of the annotations? Or maybe we should wait for the spec?

It should be safe since your implementation is backwards-compatible.
I'm okay with it if reviewers think this PR should wait for the spec.

Fznamznon · 2021-12-02T15:03:53Z

@shuoniu-intel , Is it safe to merge this one? Does FPGA compiler support new form of the annotations? Or maybe we should wait for the spec?

It should be safe since your implementation is backwards-compatible. I'm okay with it if reviewers think this PR should wait for the spec.

Please note that even though the new builtin is backward compatible for users in C++ code, this patch adds permanent generation of four new default values in the annotation string in LLVM IR, so after this patch the output is always a bit different. I was wondering if FPGA compiler will be ok with that.
I've ran extended pre commit in order to understand if some of the compiler's components are not ok with this change. Even though the tests have passed, it can be explained by lack of the tests. So I'm double checking with people.
BTW, @MrSidims , do you know if SPIR-V translator will be ok with the new LLVM IR emitted?

shuoniu-intel · 2021-12-02T15:11:30Z

Let this PR wait for the spec then. Meanwhile I'll verify if the FPGA compiler is okay with the new annotation string.

Btw this will be the new look of the annotation string output from clang and SPIR-V right?
[[ANN1:@.str[\.]*[0-9]*]] = {{.*}}{params:384}{cache-size:0}{anchor-id:-1}{target-anchor:0}{type:0}{cycle:0}

MrSidims · 2021-12-02T15:12:23Z

do you know if SPIR-V translator will be ok with the new LLVM IR emitted?

I expect, that it will map LLVM IR construct on UserSemantic decoration and translate it backwards without any problems. If it's not happening - then it's a bug in the translator.

I've ran extended pre commit in order to understand if some of the compiler's components are not ok with this change

I don't think we have OK test coverage for FPGA h/w here :(

In general, if the compiler doesn't know what to do with annotation information - the annotation intrinsics should be ignored (if seen otherwise - it's a bug in the device's backend IMO).

Fznamznon · 2021-12-02T15:22:33Z

Btw this will be the new look of the annotation string output from clang and SPIR-V right? [[ANN1:@.str[\.]*[0-9]*]] = {{.*}}{params:384}{cache-size:0}{anchor-id:-1}{target-anchor:0}{type:0}{cycle:0}

Yeah, It will look like the old variant but with default values {anchor-id:-1}{target-anchor:0}{type:0}{cycle:0} added at the end.
An example of normal emitted IR:

@.str = private unnamed_addr constant [75 x i8] c"{params:384}{cache-size:0}{anchor-id:-1}{target-anchor:0}{type:0}{cycle:0}\00", section "llvm.metadata"

@Fznamznon

This is a continuation of @Fznamznon's PR #5002. Most of the changes here are from there. According to #4980 this built-in should take four more constant integer parameters with following default values: const int32_t AnchorID = -1 const int32_t TargetAnchor = 0 const int32_t Type = 0 const int32_t Cycle = 0 The old variant is still allowed for backward compatibility. In case some parameters are not provided - default value will be emitted in annotation string. In addition to the state of PR #5002, this PR makes minor changes. It incorporates @mlychkov's code review comments there and modifies the CodeGenSYCL test to accept both opaque and non-opaque pointers.

Fznamznon requested a review from shuoniu-intel November 22, 2021 08:22

Fznamznon requested review from elizabethandrews, premanandrao and smanna12 as code owners November 22, 2021 08:22

Fznamznon added 2 commits November 23, 2021 16:22

Apply clang-format

4ba6244

Merge branch 'sycl' into upd-intel-fpga-mem

ac48e18

premanandrao reviewed Nov 29, 2021

View reviewed changes

clang/lib/CodeGen/CGBuiltin.cpp Outdated Show resolved Hide resolved

clang/lib/Sema/SemaChecking.cpp Outdated Show resolved Hide resolved

clang/lib/Sema/SemaChecking.cpp Outdated Show resolved Hide resolved

clang/test/CodeGenSYCL/intel-fpga-mem-builtin.cpp Outdated Show resolved Hide resolved

premanandrao reviewed Nov 29, 2021

View reviewed changes

clang/test/CodeGenSYCL/intel-fpga-mem-builtin.cpp Show resolved Hide resolved

clang/test/SemaSYCL/intel-fpga-mem-builtin.cpp Show resolved Hide resolved

Apply CR suggestions

6136f23

Fznamznon requested a review from premanandrao November 30, 2021 13:58

smanna12 previously approved these changes Nov 30, 2021

View reviewed changes

premanandrao previously approved these changes Nov 30, 2021

View reviewed changes

Fznamznon dismissed stale reviews from premanandrao and smanna12 via 5916950 November 30, 2021 16:35

Fix tests

5916950

premanandrao approved these changes Nov 30, 2021

View reviewed changes

smanna12 approved these changes Nov 30, 2021

View reviewed changes

mlychkov reviewed Dec 2, 2021

View reviewed changes

bader marked this pull request as draft December 2, 2021 17:08

github-actions bot added the Stale label Jun 1, 2022

premanandrao mentioned this pull request Jun 24, 2022

[SYCL][FPGA] Update __builtin_intel_fpga_mem #6357

Merged

github-actions bot closed this Jul 2, 2022

[SYCL][FPGA] Update __builtin_intel_fpga_mem #5002

[SYCL][FPGA] Update __builtin_intel_fpga_mem #5002

Uh oh!

Conversation

Fznamznon commented Nov 22, 2021

Uh oh!

Fznamznon commented Nov 22, 2021

Uh oh!

shuoniu-intel commented Nov 22, 2021

Uh oh!

Fznamznon commented Nov 29, 2021

Uh oh!

premanandrao left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

smanna12 left a comment

Choose a reason for hiding this comment

Uh oh!

premanandrao left a comment

Choose a reason for hiding this comment

Uh oh!

Fznamznon commented Nov 30, 2021

Uh oh!

Fznamznon commented Dec 2, 2021

Uh oh!

bader commented Dec 2, 2021

Uh oh!

mlychkov Dec 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shuoniu-intel commented Dec 2, 2021

Uh oh!

Fznamznon commented Dec 2, 2021

Uh oh!

shuoniu-intel commented Dec 2, 2021

Uh oh!

MrSidims commented Dec 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Fznamznon commented Dec 2, 2021

Uh oh!

Uh oh!

mlychkov Dec 2, 2021 •

edited

Loading

MrSidims commented Dec 2, 2021 •

edited

Loading