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

premanandrao · 2022-06-10T20:47:28Z

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.

…s used

premanandrao · 2022-06-10T20:48:40Z

@erichkeane and @AaronBallman, does this address the comments you had made in #6152? (Sorry that I am unable to continue the patch there.)

erichkeane · 2022-06-13T14:51:01Z

clang/lib/Sema/SemaType.cpp

@@ -2305,10 +2305,21 @@ QualType Sema::BuildBitIntType(bool IsUnsigned, Expr *BitWidth,
    return QualType();
  }

-  const TargetInfo &TI = getASTContext().getTargetInfo();
-  if (NumBits > TI.getMaxBitIntWidth()) {
+  size_t MaxBitIntWidth = getASTContext().getTargetInfo().getMaxBitIntWidth();


I wonder if instead we should be setting the aux-target in Host Mode to whatever the device triple is, and looking it up there? That would generalize this entire thing to : "if NumBits >TI.getMaxBitintWidth() && NumBits > AuxTI.getMaxBitIntWidth()"

Are you okay with this being done in a follow-up patch?

And should that change be happening upstream as well?

I'm a touch concerned as to the behavior changes we might get out of making this a part of the aux target (that is, accepting this and then doing the aux-target changes resulting in different behavior?). So I would at least want to see that patch be 'close' before accepting this.

@AaronBallman : While I think that is a GOOD idea (to set the aux target for host mode upstream), it doesn't help here since upstream doesn't have FPGA.

erichkeane · 2022-06-21T19:36:30Z

clang/lib/Sema/SemaType.cpp

@@ -2305,10 +2305,15 @@ QualType Sema::BuildBitIntType(bool IsUnsigned, Expr *BitWidth,
    return QualType();
  }

-  const TargetInfo &TI = getASTContext().getTargetInfo();
-  if (NumBits > TI.getMaxBitIntWidth()) {
+  // If the number of bits exceed the maximum bit width supported on


Hmm... I guess this would cause issues if the code is executed on the other platform, right? So we DO want to enforce the 'host' limit, but only if the 'host' bit-int gets emitted/used/etc? And vise-versa for the device? Or are we just hoping the failure is not quite silent later?

Or are we just hoping the failure is not quite silent later?

Right, I think that is what I was hoping. In this case we are in a configuration where one platform allows it but the other does not. Rather than preventing compilation here, I was relying on later phases catching it "nicely".

(In the specific case of intelfpga, which is where this can happen currently, there wouldn't be an issue later.)

That is incredibly unfortunate... We don't really have a good way of diagnosing with 'deferred diagnostics' on host unfortunately, so I suspect we're going to have to live with this.

erichkeane · 2022-06-22T12:58:14Z

clang/lib/Sema/SemaType.cpp

@@ -2305,10 +2305,15 @@ QualType Sema::BuildBitIntType(bool IsUnsigned, Expr *BitWidth,
    return QualType();
  }

-  const TargetInfo &TI = getASTContext().getTargetInfo();
-  if (NumBits > TI.getMaxBitIntWidth()) {
+  // If the number of bits exceed the maximum bit width supported on


That is incredibly unfortunate... We don't really have a good way of diagnosing with 'deferred diagnostics' on host unfortunately, so I suspect we're going to have to live with this.

elizabethandrews · 2022-06-22T14:35:24Z

clang/test/SemaSYCL/sycl-intelfpga.cpp

+// RUN: %clang_cc1 -fsycl-is-device -verify=device -fsyntax-only %s
+// RUN: %clang_cc1 -fsycl-is-host -verify=host -fsyntax-only %s
+
+// Tests that we do not issue errors for _Bitints of size greater than 128


Should we add a test where we do not diagnose since that is what this comment implies? I don't have strong preference since CodeGen tests test this anyway. So I leave it up to you.

Yeah, I added the "passing" cases in CodeGenSYCL. I don't mind adding a case that would compile cleanly here. Is a value of 215 (greater than x86_64 limit of 128 but lower than fpga limit of 2048) okay with you?

Sounds good to me

premanandrao · 2022-06-22T16:49:34Z

@hchilama, would you mind reviewing the driver changes while @mdtoguchi is on vacation? Particularly the changes in this change set ?

premanandrao · 2022-06-23T05:22:08Z

@AaronBallman, are you okay with the new changes?

AaronBallman

LGTM aside from a tiny nit (feel free to fix when landing).

clang/lib/Frontend/CompilerInstance.cpp

AaronBallman

LGTM

premanandrao · 2022-06-23T17:15:16Z

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

pvchupin · 2022-06-23T20:43:46Z

@premanandrao, please address post-commit issues on windows: https://github.com/intel/llvm/runs/7029194202?check_suite_focus=true
Tagging @hchilama, seems problem is on drivers tests. I should probably give another chance to review driver changes. :(

elizabethandrews · 2022-06-23T20:48:40Z

@premanandrao, please address post-commit issues on windows: https://github.com/intel/llvm/runs/7029194202?check_suite_focus=true Tagging @hchilama, seems problem is on drivers tests. I should probably give another chance to review driver changes. :(

Looks like it fails on Windows because test checks aux triple - "[[ARCH]]-unknown-linux-gnu"{{.*}"

// CHK-SYCL-FPGA-AUX-TRIPLE: clang{{.*}} "-cc1" "-triple" "[[ARCH]]-unknown-linux-gnu"{{.*}} "-aux-triple" "[[ARCH2]]-unknown-unknown"{{.*}} "-fsycl-is-host"

On Windows it is - "x86_64-pc-windows-msvc"

pvchupin · 2022-06-23T20:55:59Z

Thanks @elizabethandrews, feel free to submit a PR with a fix.

premanandrao · 2022-06-24T11:12:27Z

@premanandrao, please address post-commit issues on windows

PR #6355 fixes this.

premanandrao · 2022-06-24T11:12:50Z

On Windows it is - "x86_64-pc-windows-msvc"

Indeed. Local testing did not catch this.

PR #6295 modified Driver/sycl-offload.c incorrectly. It used a triple specific to Linux, and this failed on Windows. This PR fixes that by checking a wildcard pattern for the other triple values.

premanandrao added 6 commits May 13, 2022 10:15

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

ccd1d1a

…s used

Fix clang-format errors

c308ba8

Add CodeGen tests

2f91417

Fix clang-format errors

d8629a7

Address review comments

da2d0d9

Use a larger bitsize limit for fpga target

6392c0d

premanandrao requested review from a team as code owners June 10, 2022 20:47

premanandrao requested review from AaronBallman and erichkeane June 10, 2022 20:47

Fix clang-format errors

5e910ae

erichkeane reviewed Jun 13, 2022

View reviewed changes

premanandrao added 4 commits June 21, 2022 10:14

Address CR comments; pass -aux-triple to host sycl compilation

10f3a60

Fix clang-format

018546e

Fix clang-format again

a238f14

Fix test

f9aa4ef

shuoniu-intel requested a review from erichkeane June 21, 2022 19:30

erichkeane reviewed Jun 21, 2022

View reviewed changes

erichkeane previously approved these changes Jun 22, 2022

View reviewed changes

elizabethandrews previously approved these changes Jun 22, 2022

View reviewed changes

Address CR comments for Sema test

d9df48b

premanandrao dismissed stale reviews from elizabethandrews and erichkeane via d9df48b June 22, 2022 16:09

smanna12 previously approved these changes Jun 22, 2022

View reviewed changes

premanandrao requested a review from hchilama June 22, 2022 16:50

hchilama previously approved these changes Jun 22, 2022

View reviewed changes

elizabethandrews previously approved these changes Jun 22, 2022

View reviewed changes

AaronBallman previously approved these changes Jun 23, 2022

View reviewed changes

clang/lib/Frontend/CompilerInstance.cpp Outdated Show resolved Hide resolved

Address CR comments

987ba8c

premanandrao dismissed stale reviews from AaronBallman, elizabethandrews, hchilama, and smanna12 via 987ba8c June 23, 2022 13:10

smanna12 approved these changes Jun 23, 2022

View reviewed changes

AaronBallman approved these changes Jun 23, 2022

View reviewed changes

pvchupin merged commit db5f72a into intel:sycl Jun 23, 2022

premanandrao mentioned this pull request Jun 24, 2022

[SYCL] Fix Driver test after PR#6295 #6355

Merged

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

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

Uh oh!

Conversation

premanandrao commented Jun 10, 2022

Uh oh!

premanandrao commented Jun 10, 2022

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!

premanandrao commented Jun 22, 2022

Uh oh!

premanandrao commented Jun 23, 2022

Uh oh!

AaronBallman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AaronBallman left a comment

Choose a reason for hiding this comment

Uh oh!

premanandrao commented Jun 23, 2022

Uh oh!

pvchupin commented Jun 23, 2022

Uh oh!

elizabethandrews commented Jun 23, 2022

Uh oh!

pvchupin commented Jun 23, 2022

Uh oh!

premanandrao commented Jun 24, 2022

Uh oh!

premanandrao commented Jun 24, 2022

Uh oh!

Uh oh!