This is an archive of the discontinued LLVM Phabricator instance.

[Polly] Fix REQUIRES for nvptx-dependent tests
ClosedPublic

Authored by probinson on Dec 9 2022, 10:55 AM.

Details

Summary

These have been effectively disabled ever since 'nvptx' was added to
the REQUIRES clauses, because REQUIRES does not support triple checks.
The new 'target=<triple>' is supported, so switch to that scheme.
Fix up XFAIL annotations, now that these tests are actually run.

Part of the project to eliminate special handling for triples in lit
expressions.

Diff Detail

Event Timeline

probinson created this revision.Dec 9 2022, 10:55 AM
Herald added a project: Restricted Project. · View Herald Transcript
probinson requested review of this revision.Dec 9 2022, 10:55 AM

I did not investigate _why_ any of these tests failed, on the theory that subject matter experts should handle that. My goal is to clean up use (and misuse) of triple checks in the lit tests.

probinson edited the summary of this revision. (Show Details)Dec 9 2022, 11:20 AM
probinson updated this revision to Diff 482855.Dec 14 2022, 7:38 AM

double-parallel-loop.ll:
Fixed a command-line typo, but still has unexpected output,
more extensive than I'm willing to fix naively.

scalar-param-and-value-32-bit.ll:
Updated CHECK line to match current actual output.

scalar-param-and-value-use.ll:
Fixed apparent typo in the IR: check line, test now passes.

scalar-parameter-fp128.ll:
Updated "This fails" comment with current failure message.

scalar-parameter-half.ll:
Removed "This fails" comment, as the test now passes.
(It also doesn't FileCheck the output, which it probably should,
but a subject-matter expert should address that separately.)

scalar-parameter-i120.ll:
Updated "This fails" comment with current failure message.

scalar-parameter-i128.ll:
Remove "This fails" comment, as the test now passes.
(It also doesn't FileCheck the output, which it probably should,
but a subject-matter expert should address that separately.)

scalar-parameter-i3000.ll:
Updated "This fails" comment with current failure message.

scalar-parameter-i80.ll:
Updated "This fails" comment with current failure message.

scalar-parameter-ppc_fp128.ll:
Updated "This fails" comment with current failure message.

scalar-parameter-x86_fp80.ll:
Updated "This fails" comment with current failure message.

scalar-parameter.ll:
Remove redundant target datalayout clauses that prevented opt
from parsing the input; however the test still fails with
unexpected output.

probinson added a comment.EditedDec 14 2022, 7:40 AM

Made a pass over the tests trying to address the actual failures, but didn't get very far.
I'd like to commit this to unblock my lit work (described here) and let the polly experts take care of the rest of these tests.

probinson updated this revision to Diff 482859.Dec 14 2022, 7:50 AM

Update to correct diff

probinson added a subscriber: tra.Jan 9 2023, 11:07 AM

Ping. +tra based on git-log

tra added a comment.Jan 9 2023, 1:52 PM

Normally NVPTX-specific tests in clang use REQUIRES: nvptx-registered-target. There seem to be way less uses of target=... and I do not quite understand when it would be true. Would it be true when the default target matches the one specified by target=xxx, or when the compiler does have enabled back-end for that target?

Given that these tests are run w/o explicitly specifying the target, I assume you want the former and in that case this appears to be the right constraint for the tests. I am not familiar with polly, so can't comment on the tests themselves.

polly/test/GPGPU/double-parallel-loop.ll
21–26

So the idea here is to actually run the tests, but that they all are expected to fail at the moment?

Would [target=xxx] be true when the default target matches the one specified by target=xxx, or when the compiler does have enabled back-end for that target?

target=xxx would be true when the default triple matches. The check for having the backend enabled is still xxx-registered-target.

I don't know anything about these tests either, except that they came up when I searched for tests using feature names that weren't defined.

polly/test/GPGPU/double-parallel-loop.ll
21–26

This one is expected to fail, yes. Others in this directory are expected to pass. I added or removed XFAIL according to whether the tests pass or fail for me, now that they actually can be run. In a couple of cases the reason for the failure looked like a trivial mismatch, and I tweaked those to let them pass. I don't know anything about Polly or NVPTX so I can't make any more extensive changes.

tra added inline comments.Jan 9 2023, 2:34 PM
polly/test/GPGPU/double-parallel-loop.ll
21–26

Then I would suggest adding a "TODO/FIXME" to make the tests work and remove XFAIL once it's done, so it's clear that we're not doing negative testing here.

probinson updated this revision to Diff 487854.Jan 10 2023, 9:50 AM

Add "This fails today ..." notes to the tests where I added XFAIL: *

probinson marked an inline comment as done.Jan 10 2023, 9:52 AM
probinson added inline comments.
polly/test/GPGPU/double-parallel-loop.ll
21–26

The idiom in these tests appears to be "This test fails today (reason)" so I added comments in that form to the tests where I added XFAIL.

probinson marked an inline comment as done.Jan 13 2023, 10:34 AM

@tra can I get your okay on this?

tra accepted this revision.Jan 13 2023, 10:49 AM
tra added inline comments.
polly/test/GPGPU/double-parallel-loop.ll
23

Nit: I'd move REQUIRES to the top of the file.

This revision is now accepted and ready to land.Jan 13 2023, 10:49 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJan 13 2023, 11:28 AM
probinson added inline comments.Jan 13 2023, 11:28 AM
polly/test/GPGPU/double-parallel-loop.ll
23

I hear you, but the style of these tests puts RUN first, so I'm going to stick with that.