This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP] Add Error Handling for Conflicting Pointer Sizes for Target Offload
ClosedPublic

Authored by jhuber6 on Sep 30 2020, 10:15 AM.

Details

Summary

This patch adds an error to Clang that detects if OpenMP offloading is used between two architectures with incompatible pointer sizes. This ensures that the data mapping can be done correctly and solves an issue in code generation generating the wrong size pointer.

Diff Detail

Event Timeline

jhuber6 created this revision.Sep 30 2020, 10:15 AM
jhuber6 requested review of this revision.Sep 30 2020, 10:15 AM
This revision is now accepted and ready to land.Sep 30 2020, 10:36 AM
jhuber6 closed this revision.Sep 30 2020, 11:06 AM

Committed in rG9d2378b59150, forgot to update the commit message.

jhuber6 updated this revision to Diff 295949.Oct 2 2020, 7:13 PM

Adding an extra substitution in lit to solve the problem of automatically populating the test files with incompatible architectures.

Herald added a project: Restricted Project. · View Herald TranscriptOct 2 2020, 7:13 PM
jhuber6 reopened this revision.Oct 2 2020, 7:14 PM

Reopening for now.

This revision is now accepted and ready to land.Oct 2 2020, 7:14 PM
jhuber6 updated this revision to Diff 296081.Oct 4 2020, 3:14 PM

Changed the lit substitution to be for fixing this problem specifically. It made the tests too unreadable and wasn't a good solution since it didn't detect 16 bit architectures anyway.

jdoerfert accepted this revision.Oct 5 2020, 7:16 AM

LGTM, a new triple makes sense here, we don't support arbitrary combinations.

This revision was landed with ongoing or failed builds.Oct 5 2020, 8:03 AM
This revision was automatically updated to reflect the committed changes.
jhuber6 updated this revision to Diff 296204.Oct 5 2020, 9:49 AM

Changing method of determining architecture.

jhuber6 updated this revision to Diff 296748.Oct 7 2020, 11:16 AM

Removing the architecture detection from lit. Simply changing the tests to use specific architectures, in this case x86_64 and aarch64.

ABataev added inline comments.
clang/lib/Frontend/CompilerInvocation.cpp
3218–3223

Maybe do something like `Triple::getArchPointerBitWidth(T.getArch()) != Triple::getArchPointerBitWidth(TT.getArch())"?

jhuber6 added inline comments.Oct 7 2020, 11:27 AM
clang/lib/Frontend/CompilerInvocation.cpp
3218–3223

That was my first thought but that function is private to the Triple class so I just went with this brute force method. The file has this comment for the justification.

/// Test whether the architecture is 64-bit
///
/// Note that this tests for 64-bit pointer width, and nothing else. Note
/// that we intentionally expose only three predicates, 64-bit, 32-bit, and
/// 16-bit. The inner details of pointer width for particular architectures
/// is not summed up in the triple, and so only a coarse grained predicate
/// system is provided.
ABataev added inline comments.Oct 7 2020, 11:32 AM
clang/lib/Frontend/CompilerInvocation.cpp
3218–3223

Would it be better to convert the results of isArch...Bit() to enum and compare enums then? Like:

enum {Arch16bit, Arch32bit, Arch64bit} TArch, TTArch;

if (T.isArch16Bit())
  Tarch = Arch16Bit;
else if ...
...
// Same for TT.
if (TArch != TTArch)
...
jhuber6 added inline comments.Oct 7 2020, 11:35 AM
clang/lib/Frontend/CompilerInvocation.cpp
3218–3223

You'd end up with a similar situation right, having about 6 conditionals manually checking check predicate right?

if (T.isArch16Bit())
else if (T.isArch32Bit())
else if (T.isArch64Bit())
if (TT.isArch16Bit())
else if (TT.isArch32Bit())
else if (TT.isArch64Bit())
ABataev added inline comments.Oct 7 2020, 11:48 AM
clang/lib/Frontend/CompilerInvocation.cpp
3218–3223

Yes, but it may be easier to maintain in the future, say when 128 or 256 bits architectures are added? :)
Plus, the last one should not be else if, just:

else {
  assert(T.isArch64Bit() && "Expected 64 bits arch");
  TArch = ...;
}
jhuber6 added inline comments.Oct 7 2020, 11:58 AM
clang/lib/Frontend/CompilerInvocation.cpp
3218–3223

Something like this sufficient?

enum ArchPtrSize {Arch16Bit, Arch16Bit, Arch16Bit};
auto GetArchPtrSize[](llvm::Triple &T) {
    if (T.isArch16Bit())
        return Arch16Bit;
    else if (T.isArch32Bit())
        return Arch32Bit;
    else
        return Arch64Bit;
};

if (GetArchPtrSize(T) != GetArchPtrSize(TT)) { ... }
ABataev added inline comments.Oct 7 2020, 12:04 PM
clang/lib/Frontend/CompilerInvocation.cpp
3218–3223
  1. Make it static.
  2. GetArchPtrSize->getArchPtrSize.
  3. No need to use else if the substatement is return:
if (16bit)
  return Arch16Bit;
if (32Bit)
  return Arch32Bit;
assert(64Bit && "expected 64 bit arch");
return Arch64Bit;
  1. Move enum to anonymous namespace.
jhuber6 updated this revision to Diff 296778.Oct 7 2020, 1:14 PM

Changed method for comparing the architectures. Enum is local to the loop so it shouldn't pollute the namespace.

LG with a nit.

clang/lib/Frontend/CompilerInvocation.cpp
3195

const Triple &

jhuber6 updated this revision to Diff 296783.Oct 7 2020, 1:24 PM

Okay, I'll push it after this finishes its build and see if buildbot sends me angry emails again.