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.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Unit Tests
| Time | Test | |
|---|---|---|
| 880 ms | linux > libarcher.parallel::parallel-nosuppression.c |
Event Timeline
Comment Actions
Adding an extra substitution in lit to solve the problem of automatically populating the test files with incompatible architectures.
Comment Actions
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.
Comment Actions
Removing the architecture detection from lit. Simply changing the tests to use specific architectures, in this case x86_64 and aarch64.
| clang/lib/Frontend/CompilerInvocation.cpp | ||
|---|---|---|
| 3209–3214 | Maybe do something like `Triple::getArchPointerBitWidth(T.getArch()) != Triple::getArchPointerBitWidth(TT.getArch())"? | |
| clang/lib/Frontend/CompilerInvocation.cpp | ||
|---|---|---|
| 3209–3214 | 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. | |
| clang/lib/Frontend/CompilerInvocation.cpp | ||
|---|---|---|
| 3209–3214 | 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)
... | |
| clang/lib/Frontend/CompilerInvocation.cpp | ||
|---|---|---|
| 3209–3214 | 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()) | |
| clang/lib/Frontend/CompilerInvocation.cpp | ||
|---|---|---|
| 3209–3214 | Yes, but it may be easier to maintain in the future, say when 128 or 256 bits architectures are added? :) else {
assert(T.isArch64Bit() && "Expected 64 bits arch");
TArch = ...;
} | |
| clang/lib/Frontend/CompilerInvocation.cpp | ||
|---|---|---|
| 3209–3214 | 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)) { ... } | |
| clang/lib/Frontend/CompilerInvocation.cpp | ||
|---|---|---|
| 3209–3214 |
if (16bit) return Arch16Bit; if (32Bit) return Arch32Bit; assert(64Bit && "expected 64 bit arch"); return Arch64Bit;
| |
Comment Actions
Changed method for comparing the architectures. Enum is local to the loop so it shouldn't pollute the namespace.
Comment Actions
Okay, I'll push it after this finishes its build and see if buildbot sends me angry emails again.
const Triple &