[SYCL] Allow _Bitint of size greater than 128 bits when -fintelfpga i… #6152

premanandrao · 2022-05-13T17:37:09Z

…s used

elizabethandrews

Can you add a CodeGen test to ensure IR is generated as expected for 128 bit?

elizabethandrews · 2022-05-13T17:46:05Z

clang/lib/Sema/SemaType.cpp

@@ -2306,7 +2306,7 @@ QualType Sema::BuildBitIntType(bool IsUnsigned, Expr *BitWidth,
  }

  const TargetInfo &TI = getASTContext().getTargetInfo();
-  if (NumBits > TI.getMaxBitIntWidth()) {
+  if (NumBits > TI.getMaxBitIntWidth() && !Context.getLangOpts().IntelFPGA) {


Shouldn't there be some code handling the larger size for this target? Is just removing the error enough?

I believe it is enough. The actual handling is done by the backend or via library calls.

Actually, this is not enough. We need to make sure we don't emit anything too large for the LLVM backend.

IMO, the RIGHT place for this is probably to get the proper target-info for the FPGA to change its answer to 'getMaxBitIntWidth' based on the FPGA status to the LLVM max value.

Actually, this is not enough. We need to make sure we don't emit anything too large for the LLVM backend.

IMO, the RIGHT place for this is probably to get the proper target-info for the FPGA to change its answer to 'getMaxBitIntWidth' based on the FPGA status to the LLVM max value.

If we go with this way in any following patch, 2048 is a reasonable upper-bound limit for the FPGA target.

To clarify: My concern is this: https://llvm.org/doxygen/classllvm_1_1IntegerType.html#af13295ff815293f6c5708ebdbae1338da1ccefdf8a7414a6829f888e5071e0379

if there is a reasonable max for the FPGA target, we should definitely do that too.

premanandrao · 2022-05-13T20:35:52Z

Can you add a CodeGen test to ensure IR is generated as expected for 128 bit?

I have added one for the host and one for the device. These are just rudimentary tests to see that nothing crashes; they are not meant for comprehensive testing of all _BitInt operations.

keryell · 2022-05-13T22:26:50Z

@Ralender this is to generalize.

clang/lib/Driver/ToolChains/Clang.cpp

shuoniu-intel · 2022-05-16T13:24:35Z

When I tested >128 BitInt division with sycl/rel to bypass this err_bit_int_max_size diagnose in sycl/trunk, I got another error - "Unsupported library call operation!" in the clang++ -cc1 -triple x86_64-unknown-linux-gnu ... compile of the non-emulation flow and in the opencl-aot ... compile of the emulation flow. However, I don't see such diagnose string in DiagnosticSemaKinds.td.

So I wonder if this error still exists in sycl/trunk and needs to be handled in this patch as well? I cannot verify this myself, because the err_bit_int_max_size diagnose in the clang++ -cc1 -triple spir64_fpga-unknown-unknown ... compile currently blocks any later diagnoses in sycl/trunk.

premanandrao · 2022-05-16T13:55:32Z

When I tested >128 BitInt division with sycl/rel to bypass this err_bit_int_max_size diagnose in sycl/trunk, I got another error - "Unsupported library call operation!" in the clang++ -cc1 -triple x86_64-unknown-linux-gnu ... compile of the non-emulation flow and in the opencl-aot ... compile of the emulation flow. However, I don't see such diagnose string in DiagnosticSemaKinds.td.

So I wonder if this error still exists in sycl/trunk and needs to be handled in this patch as well? I cannot verify this myself, because the err_bit_int_max_size diagnose in the clang++ -cc1 -triple spir64_fpga-unknown-unknown ... compile currently blocks any later diagnoses in sycl/trunk.

Do you have a test case that shows the error above? I can verify that with the current sources and my patch.

shuoniu-intel · 2022-05-16T13:59:25Z

Thank you :) I can upload my test case to the Jira.

Do you have a test case that shows the error above? I can verify that with the current sources and my patch.

premanandrao · 2022-05-16T15:15:06Z

Thank you :) I can upload my test case to the Jira.

I am moving this PR to draft status until the post-link errors reported by @shuoniu-intel are resolved.

premanandrao · 2022-06-01T17:27:32Z

I am moving this PR to draft status until the post-link errors reported by @shuoniu-intel are resolved.

These appear to be resolved now.

mdtoguchi

ok for Driver

smanna12

LGTM.

premanandrao · 2022-06-02T14:01:56Z

@intel/llvm-gatekeepers, this is ready for a merge.

AaronBallman · 2022-06-02T14:07:26Z

clang/lib/Sema/SemaType.cpp

@@ -2306,7 +2306,7 @@ QualType Sema::BuildBitIntType(bool IsUnsigned, Expr *BitWidth,
  }

  const TargetInfo &TI = getASTContext().getTargetInfo();
-  if (NumBits > TI.getMaxBitIntWidth()) {
+  if (NumBits > TI.getMaxBitIntWidth() && !Context.getLangOpts().IntelFPGA) {


This change looks wrong to me -- LLVM has a max bit width size that we must ensure the user stays within the bounds of. Passing the intel FPGA flag won't make a bit of difference in that case, it'll just crash in the backend.

AaronBallman · 2022-06-02T14:09:14Z

clang/test/CodeGenSYCL/no-opaque-ptrs-sycl-host-intelfpga-bitint.cpp

+// is allowed when -fintelfpga is enabled.
+
+// CHECK: define{{.*}} void @_Z3fooDB211_S_(i211* {{.*}} sret(i211) align 8 %agg.result, i211* {{.*}} byval(i211) align 8 %[[ARG1:[0-9]+]], i211* {{.*}} byval(i211) align 8 %[[ARG2:[0-9]+]])
+signed _BitInt(211) foo(signed _BitInt(211) a, signed _BitInt(211) b) {


This is a good test, now try it with _BitInt(123456). Do the results still seem reasonable? (Basically, we should be testing the edge cases much more heavily than we seem to be doing.)

AaronBallman · 2022-06-02T14:10:56Z

It seems this was merged before I could post my comments -- please address them in a follow up (or revert if the follow up is going to take significant time).

premanandrao · 2022-06-02T14:17:19Z

It seems this was merged before I could post my comments -- please address them in a follow up (or revert if the follow up is going to take significant time).

Thanks for the comments @AaronBallman and @erichkeane. I will revert it for now and then reapply the changes you suggest.

bader · 2022-06-02T14:23:45Z

Let me handle the revert for you.

…elfpga is used (#6152)" This reverts commit 51f2ea3.

…elfpga is used (#6152)" (#6232) This reverts commit 51f2ea3.

bader · 2022-06-02T14:24:42Z

Let me handle the revert for you.

See #6232.

…s used (#6295) This revives the patch for #6152 (which was reverted in #6232). In this patch, the new change is that there is a new target for spir64_fpga and the maximum bitsize limit for that target is 2048. Unfortunately, for the host, there is now an explicit check in Sema for the same bitsize. When -fintefpga is specified with -fsycl, we allow a maximum bitsize of 2048.

[SYCL] Allow _Bitint of size greater than 128 bits when -fintelfpga i…

ccd1d1a

…s used

premanandrao requested review from a team as code owners May 13, 2022 17:37

Fix clang-format errors

c308ba8

elizabethandrews reviewed May 13, 2022

View reviewed changes

Add CodeGen tests

2f91417

Fix clang-format errors

d8629a7

mdtoguchi reviewed May 13, 2022

View reviewed changes

clang/lib/Driver/ToolChains/Clang.cpp Show resolved Hide resolved

premanandrao marked this pull request as draft May 16, 2022 15:15

Address review comments

da2d0d9

premanandrao marked this pull request as ready for review June 1, 2022 17:27

mdtoguchi approved these changes Jun 1, 2022

View reviewed changes

smanna12 approved these changes Jun 1, 2022

View reviewed changes

smanna12 requested a review from elizabethandrews June 1, 2022 17:50

elizabethandrews approved these changes Jun 1, 2022

View reviewed changes

bader merged commit 51f2ea3 into intel:sycl Jun 2, 2022

AaronBallman requested changes Jun 2, 2022

View reviewed changes

bader added a commit that referenced this pull request Jun 2, 2022

Revert "[SYCL] Allow _Bitint of size greater than 128 bits when -fint…

565664c

…elfpga is used (#6152)" This reverts commit 51f2ea3.

bader mentioned this pull request Jun 2, 2022

Revert "[SYCL] Allow _Bitint of size greater than 128 bits when -fintelfpga i…" #6232

Merged

bader added a commit that referenced this pull request Jun 2, 2022

Revert "[SYCL] Allow _Bitint of size greater than 128 bits when -fint…

c9cdf25

…elfpga is used (#6152)" (#6232) This reverts commit 51f2ea3.

premanandrao mentioned this pull request Jun 10, 2022

[SYCL] Allow _Bitint of size greater than 128 bits when -fintelfpga is used #6295

Merged

[SYCL] Allow _Bitint of size greater than 128 bits when -fintelfpga i… #6152

[SYCL] Allow _Bitint of size greater than 128 bits when -fintelfpga i… #6152

Uh oh!

Conversation

premanandrao commented May 13, 2022

Uh oh!

elizabethandrews left a comment

Choose a reason for hiding this comment

Uh oh!

elizabethandrews May 13, 2022

Choose a reason for hiding this comment

Uh oh!

premanandrao May 13, 2022

Choose a reason for hiding this comment

Uh oh!

erichkeane Jun 2, 2022

Choose a reason for hiding this comment

Uh oh!

shuoniu-intel Jun 2, 2022

Choose a reason for hiding this comment

Uh oh!

erichkeane Jun 2, 2022

Choose a reason for hiding this comment

Uh oh!

premanandrao commented May 13, 2022

Uh oh!

keryell commented May 13, 2022

Uh oh!

Uh oh!

shuoniu-intel commented May 16, 2022

Uh oh!

premanandrao commented May 16, 2022

Uh oh!

shuoniu-intel commented May 16, 2022

Uh oh!

premanandrao commented May 16, 2022

Uh oh!

premanandrao commented Jun 1, 2022

Uh oh!

mdtoguchi left a comment

Choose a reason for hiding this comment

Uh oh!

smanna12 left a comment

Choose a reason for hiding this comment

Uh oh!

premanandrao commented Jun 2, 2022

Uh oh!

AaronBallman Jun 2, 2022

Choose a reason for hiding this comment

Uh oh!

AaronBallman Jun 2, 2022

Choose a reason for hiding this comment

Uh oh!

AaronBallman commented Jun 2, 2022

Uh oh!

premanandrao commented Jun 2, 2022

Uh oh!

bader commented Jun 2, 2022

Uh oh!

bader commented Jun 2, 2022

Uh oh!

Uh oh!