Page MenuHomePhabricator

[CLANG] Fix missing error for use of 128-bit integer inside SPIR64 device code.
ClosedPublic

Authored by jyu2 on Dec 1 2020, 5:21 PM.

Details

Summary

Emit error for use of 128-bit integer inside device code had been
already implemented in https://reviews.llvm.org/D74387. However,
the error is not emitted for SPIR64, due for SPIR64 hasInt128Type
return true.

hasInt128Type: is also used to control generation of certain 128-bit
predefined macros, initializer predefined 128-bit integer types and
build 128-bit ArithmeticTypes. Except predefined macros, only the
device target is considered, since error only emit when 128-bit
integer is used inside device code, the host target (auxtarget) also
needs to be considered.

The change address:

  1. (SPIR.h) Correct hasInt128Type() for SPIR targets.
  2. Sema.cpp and SemaOverload.cpp: Add additional check to consider host target(auxtarget) when call to hasInt128Type. So that int128_t and int128() are allowed to avoid error when they used outside device code.
  3. SemaType.cpp: add check for SYCLIsDevice to delay the error message. The error will be emitted if the use of 128-bit integer in the device code.

Diff Detail

Event Timeline

jyu2 created this revision.Dec 1 2020, 5:21 PM
jyu2 requested review of this revision.Dec 1 2020, 5:21 PM
jdoerfert requested changes to this revision.Dec 1 2020, 11:20 PM
jdoerfert added inline comments.
clang/lib/Basic/Targets/SPIR.h
108

Not virtual but override.

clang/lib/Sema/Sema.cpp
241

I don't understand why this (and the changes below) are necessary. Host and device compilation are separate. This should not be any different to CUDA, HIP, or OpenMP offload which seem not to require this.

This revision now requires changes to proceed.Dec 1 2020, 11:20 PM
jyu2 added inline comments.Dec 2 2020, 8:54 AM
clang/lib/Basic/Targets/SPIR.h
108

Thank you. I will change that.

clang/lib/Sema/Sema.cpp
241

As far as I can know, in real compile, the auxtarget should be also set for example SYCL. May be that is only for our compiler. I am not sure what do you mean by it should same with CUDA, HIP...? Do you mean, CUDA(or HIP...) target should also not support 128-bit integer? If so, I don't see any error emit for CUDA when I try with nvptx64-nvidia-cuda.

Thanks.

So, NVPTX seems to do something sensible with i128, though I haven't done rigorous testing.

clang/lib/Sema/Sema.cpp
241

I'm not saying auxtarget is not set. I'm trying to understand why this is necessary. Why would you initialize __int128_t if your target doesn't support it? What happens if you only include the SPIR.h change?

jyu2 added inline comments.Dec 2 2020, 9:45 AM
clang/lib/Sema/Sema.cpp
241

Without this change you will see error:

j3.c:2:15: error: unknown type name 'uint128_t'
typedef const
uint128_t megeType;

bash-4.4$ cat j3.c

typedef const __uint128_t megeType;

Without change in SemaOverload.c:
you will see error:
x.cpp:3:14: error: use of overloaded operator '==' is ambiguous (with operand types 'struct X' and '__int128')

bool a = x == __int128(0);
         ~ ^  ~~~~~~~~~~~

bash-4.4$ cat x.cpp
namespace PR12964 {

struct X { operator  __int128() const; } x;
bool a = x == __int128(0);

}

jyu2 added inline comments.Dec 2 2020, 12:55 PM
clang/lib/Sema/Sema.cpp
241

Just one more point, the errors only need to be emitted for 128-bit integer used inside device code. The test case above 128-bit integer is not used in device code, so we don't want to emit error for it.

Thanks.
Jennifer

Still unsure if we should also error out for NVPTX but that is a different story. Looks OK from my side, assuming you address the earlier comment.

Maybe someone else should accept though.

clang/lib/Sema/Sema.cpp
241

I see. You want the types to be there so the diagnostic says the types are not available. OK, fine by me.

jyu2 updated this revision to Diff 309067.Dec 2 2020, 2:54 PM
jyu2 retitled this revision from Fix missing error for use of 128-bit integer inside SPIR64 device code. to [CLANG] Fix missing error for use of 128-bit integer inside SPIR64 device code..

Fix problem with @jdoerfert pointed where using bool hasInt128Type() const override { return false; } instead.

jyu2 added inline comments.Dec 2 2020, 3:03 PM
clang/lib/Sema/Sema.cpp
241

Thank you so much for your time for review this!!!

jyu2 added a comment.Dec 2 2020, 4:30 PM

Still unsure if we should also error out for NVPTX but that is a different story. Looks OK from my side, assuming you address the earlier comment.

With this change if NVPTX need diagnostic for use of 128-bit integer, adding "bool hasInt128Type() const override { return false; }" in NVPTX.h is all needed.

Maybe someone else should accept though.

Do you have suggestion whom I may contact for acceptance? We have customer needs for this... Thank you in advance. :-)

Still unsure if we should also error out for NVPTX but that is a different story. Looks OK from my side, assuming you address the earlier comment.

With this change if NVPTX need diagnostic for use of 128-bit integer, adding "bool hasInt128Type() const override { return false; }" in NVPTX.h is all needed.

Maybe someone else should accept though.

Do you have suggestion whom I may contact for acceptance? We have customer needs for this... Thank you in advance. :-)

You have the right people as reviewers, it sometimes need a few days. You can ping it after a week without progress.

aaron.ballman accepted this revision.Dec 3 2020, 8:21 AM

Still unsure if we should also error out for NVPTX but that is a different story. Looks OK from my side, assuming you address the earlier comment.

With this change if NVPTX need diagnostic for use of 128-bit integer, adding "bool hasInt128Type() const override { return false; }" in NVPTX.h is all needed.

Maybe someone else should accept though.

Do you have suggestion whom I may contact for acceptance? We have customer needs for this... Thank you in advance. :-)

You have the right people as reviewers, it sometimes need a few days. You can ping it after a week without progress.

FWIW, the changes LGTM but I don't know enough about the domain to know the answer for NVPTX. That said, this is still good incremental progress as-is.

Still unsure if we should also error out for NVPTX but that is a different story. Looks OK from my side, assuming you address the earlier comment.

With this change if NVPTX need diagnostic for use of 128-bit integer, adding "bool hasInt128Type() const override { return false; }" in NVPTX.h is all needed.

Maybe someone else should accept though.

Do you have suggestion whom I may contact for acceptance? We have customer needs for this... Thank you in advance. :-)

You have the right people as reviewers, it sometimes need a few days. You can ping it after a week without progress.

FWIW, the changes LGTM but I don't know enough about the domain to know the answer for NVPTX. That said, this is still good incremental progress as-is.

Leave NVPTX out for now. As I said, it looks like the backend does the right thing so it is "legal" to have i128.

jyu2 added a comment.Dec 4 2020, 11:20 AM

Thanks @aaron.ballman to accept my change!!!
Hi @jdoerfert,
Thank you for your review.
it seems you still has request change for for this. I did address your earlier comment in SPIR.h and upload the change. Is there anything else I need to change?

Thanks.
Jennifer

This revision is now accepted and ready to land.Dec 4 2020, 1:10 PM
jyu2 closed this revision.Dec 9 2020, 10:00 AM

commit by rGf8d5b49c786f: Fix missing error for use of 128-bit integer inside SPIR64 device code.