Repository navigation
Conversation
elizabethandrews
left a comment
There was a problem hiding this comment.
This is primarily a Driver Change with just a warning group declared in FE. I defer to @mdtoguchi for review. I agree that the current proposed warning is too verbose.
… bviyer-make-better-warning
|
|
||
| for (const InputInfo &II : Inputs) | ||
| if (II.isFilename() && | ||
| isa_and_nonnull<OffloadWrapperJobAction>(II.getAction())) { |
There was a problem hiding this comment.
This is good for the old offload model. We should also look into a solution for the new model - I would expect that to be more in the clang-linker-wrapper space.
There was a problem hiding this comment.
Can we do that in a separate PR?
There was a problem hiding this comment.
Yup, this can be done in a separate PR.
| static constexpr uint64_t HugeDeviceCodeWarnThreshold = 1ull << 30; // 1GiB | ||
|
|
||
| // Sums the size of only those link inputs that carry wrapped device code, | ||
| // i.e. skips host objects, static archives, and other linker inputs that |
There was a problem hiding this comment.
the comment on static archives isn't true - as static archives should be queried as well, as they can be 'fat archives'. But that query happens in a different area so the device objects from any static archives should already be processed along with the other device binaries.
| "clang LLVM compiler", InMemoryFileSystem); | ||
| TheDriver.setCheckInputsExist(false); | ||
| std::unique_ptr<Compilation> C(TheDriver.BuildCompilation( | ||
| {"/usr/bin/clang++", "-fsycl", "-nostdlib", "foo.cpp", "-o", "foo.out"})); |
There was a problem hiding this comment.
We probably don't want the link step to occur when the warning is triggered. Adding -Werror=large-device-kernel will prevent it from running the link.
|
|
||
| // RUN: rm -f %t-big.o | ||
|
|
||
| // NO-WARN-NOT: total link input size |
There was a problem hiding this comment.
Missing tests that check that -flink-huge-device-code and -Wno-large-device-kernel prevent the warning from being emitted.
There was a problem hiding this comment.
I may have commented here to add these checks - but given that the originating objects don't have any device code in the first place - the additional tests aren't going to be doing anything here and will just be spinning compilation time on large inputs.
| // RUN: %python -c "open(r'%t-big.o', 'wb').truncate(1100000000)" | ||
| // RUN: %python -c "open(r'%t-small.o', 'wb').truncate(1000)" | ||
|
|
||
| // RUN: %clangxx -fsycl %t-big.o %t-small.o -o %t.out > %t-warn.log 2>&1 || true |
There was a problem hiding this comment.
| // RUN: %clangxx -fsycl %t-big.o %t-small.o -o %t.out > %t-warn.log 2>&1 || true | |
| // RUN: %clangxx -fsycl %t-big.o %t-small.o -o %t.out > %t-warn.log 2>&1 |
No expectation for this command to fail is there? I don't think the || true is necessary.
| // RUN: %clangxx -fsycl -fuse-ld=/bin/true %t-big.o %t-small.o -o %t.out > %t-warn.log 2>&1 | ||
| // RUN: FileCheck --check-prefix=NO-WARN --allow-empty %s < %t-warn.log | ||
|
|
||
| // RUN: %clangxx -fsycl -fuse-ld=/bin/true -flink-huge-device-code %t-big.o %t-small.o -o %t.out > %t-flink.log 2>&1 | ||
| // RUN: FileCheck --check-prefix=NO-WARN --allow-empty %s < %t-flink.log | ||
|
|
||
| // RUN: %clangxx -fsycl -fuse-ld=/bin/true -Wno-large-device-kernel %t-big.o %t-small.o -o %t.out > %t-wno.log 2>&1 | ||
| // RUN: FileCheck --check-prefix=NO-WARN --allow-empty %s < %t-wno.log |
There was a problem hiding this comment.
I would expect these tests to fail in the new offload model setting. Probably should add --no-offload-new-driver to these command lines.
| // short-circuit instead of actually invoking the (nonexistent, in this | ||
| // test) linker -- that's what we want, since the point here is just to |
There was a problem hiding this comment.
Comment says that the linker is non-existent. Probably not a true statement. Should just say we don't want to invoke the linker.
| EXPECT_TRUE(DiagConsumer->Msgs.empty()); | ||
| EXPECT_TRUE(DiagConsumer->Errors.empty()); |
There was a problem hiding this comment.
These will fail if any diagnostic is emitted. Probably should tighten this up to check for more of the specific diagnostic messaging (not) expected.
| EXPECT_TRUE(DiagConsumer->Msgs.empty()); | ||
| EXPECT_TRUE(DiagConsumer->Errors.empty()); |
There was a problem hiding this comment.
Same comment as below in regards to tightening up the diagnostic to not be emitted.
Large device code can push the 32-bit PC relative relocations past their 2GB reach. this causes host linker to fail with errors such as "relocation truncated to fit." In this PR, such cases are dignosed at linked time via
-Wlarge-device-kernelflag and providing user suggestion to use theflink-huge-device-codeas a fix.