Details
Diff Detail
Event Timeline
llvm/tools/opt/opt.cpp | ||
---|---|---|
602 | Should we sanitize that the new PM is enabled if PassPlugins is provided and error otherwise? |
llvm/tools/opt/opt.cpp | ||
---|---|---|
602 | Thanks. Done. |
I got many failures in poly.
Seems it uses both -load and -load-pass-plugin with legacy PM passes?
Here's one:
BUILD FAILED: failed 1054 unexpected failures (failure) Step 6 (check_polly) failure: 1054 unexpected failures (failure) ******************** TEST 'Polly :: ScheduleOptimizer/pattern-matching-based-opts_14.ll' FAILED ******************** Script: -- : 'RUN: at line 1'; /home/worker/buildbot-workers/polly-x86_64-gce1/rundir/llvm.obj/bin/opt -load /home/worker/buildbot-workers/polly-x86_64-gce1/rundir/llvm.obj/lib/LLVMPolly.so -load-pass-plugin /home/worker/buildbot-workers/polly-x86_64-gce1/rundir/llvm.obj/lib/LLVMPolly.so -polly-process-unprofitable -polly-remarks-minimal -polly-use-llvm-names -polly-import-jscop-dir=/home/worker/src/llvm-project/polly/test/ScheduleOptimizer -polly-codegen-verify -polly-import-jscop -polly-opt-isl -polly-target-throughput-vector-fma=1 -polly-target-latency-vector-fma=8 -polly-target-1st-cache-level-associativity=8 -polly-target-2nd-cache-level-associativity=8 -polly-target-1st-cache-level-size=32768 -polly-target-vector-register-bitwidth=256 -polly-target-2nd-cache-level-size=262144 -polly-import-jscop-postfix=transformed -polly-codegen -S < /home/worker/src/llvm-project/polly/test/ScheduleOptimizer/pattern-matching-based-opts_14.ll | /home/worker/buildbot-workers/polly-x86_64-g ce1/rundir/llvm.obj/bin/FileCheck /home/worker/src/llvm-project/polly/test/ScheduleOptimizer/pattern-matching-based-opts_14.ll -- Exit Code: 2 Command Output (stderr): -- /home/worker/buildbot-workers/polly-x86_64-gce1/rundir/llvm.obj/bin/opt: load-pass-plugin specified with legacy PM. FileCheck error: '<stdin>' is empty. FileCheck command line: /home/worker/buildbot-workers/polly-x86_64-gce1/rundir/llvm.obj/bin/FileCheck /home/worker/src/llvm-project/polly/test/ScheduleOptimizer/pattern-matching-based-opts_14.ll
@Meinersbur Hi Michael. I see this in polly's lit config file:
config.substitutions.append(('%loadPolly', '-load ' + config.polly_lib_dir + '/LLVMPolly@LLVM_SHLIBEXT@' + ' -load-pass-plugin ' + config.polly_lib_dir + '/LLVMPolly@LLVM_SHLIBEXT@'
The -load-pass-plugin was added in D45484, but the follow up change D45493 never landed.
From my understanding, -load and -load-pass-plugin are disjoint (one is for legacy PM and one for NPM) by design.
Since you cannot run both legacy and new PM, the only reason I think you need both -load and -load-pass-plugin is to be able to register polly's options before they're processed.
Which is the problem this PR addresses.
Should we remove -load-pass-plugin from %loadPolly for now, until polly switches to using NPM (i'm guessing it's using legacy PM because the new diagnostic i'm adding is being triggered by polly's tests) ?
Seems it uses both -load and -load-pass-plugin with legacy PM passes?
It was written with the assumption that -load and load-pass-plugin means "make the passes from the .so register in the PM". As such, they (should have) no effect until actually used (-load can do other things than register pass plugins). Only allowing the option corresponding to the PM actually used may help reduce user confusion, but if consequent it should also give an error if no pass from the loaded plugin was used.
Moving Polly completely to the NPM will take a while (some aspects of the NPM are still not compatible with pass plugins, or work differently needing to redesign a lot of regression tests). D45493 only takes account for the few Polly that had equivalent NPM passes by then.
If you really want that error, I suggest to introduce a new %loadPolly_NPM (or similar name) that loads the NPM plugin, and %loadPolly to only load the legacy PM plugin.
llvm/test/Feature/load_extension.ll | ||
---|---|---|
25–27 | Can you move all the RUN lines together at the beginning of the file? It is surprising to find another RUN line after e.g. having adapted the others. |
but if consequent it should also give an error if no pass from the loaded plugin was used.
Agreed. However, how can I tell if this occurs? Is there a simple way to check that?
If you really want that error, I suggest to introduce a new %loadPolly_NPM (or similar name) that loads the NPM plugin, and %loadPolly to only load the legacy PM plugin.
%loadPolly is currently used in all polly testcases. Does this require touching every polly testcase that uses %loadPolly? Is there an easy way to tell (by inspection) if a testcase is calling the NPM passes of polly?
%loadPolly would only need to be replaced with %loadPolly_NPM in test cases using the NPM. There are not that many yet. You get a list of test cases using the NPM by removing -load-pass-plugin from %loadPolly and run ninja check-polly-tests (using LLVM_POLLY_LINK_INTO_TOOLS=OFF). Those are also the RUN-lines that would need %loadPolly_NPM instead of %loadPolly. Alternatively, grep for RUN-lines with -passes= option.
Fixed failing polly tests by adding a NPM macro %loadNPMPolly.
@Meinersbur Thanks for the review. Please let me know if the cfg changes are ok before I commit the changes.
Could you rebase the patch? The premerge-check's patch application failed: https://buildkite.com/llvm-project/diff-checks/builds/94599
I did rebase it (on 12a2f7494e74) before posting. The premerge-check error seems something bogus?:
$ trap 'kill -- $$' INT TERM QUIT; pip install -q -r scripts/requirements.txt scripts/apply_patch.sh INFO repository exist, will reuse INFO working dir /var/lib/buildkite-agent/builds/llvm-project-fork INFO Syncing local, origin and upstream... INFO Planning to apply in order: INFO https://reviews.llvm.org/D121566?id=416182 INFO D121566#416182 commit 12a2f7494e745eb4c90133ea17cadac3a8eb8d07 exists INFO creating branch phab-diff-416182 at 12a2f7494e745eb4c90133ea17cadac3a8eb8d07 INFO Base branch revision is 12a2f7494e745eb4c90133ea17cadac3a8eb8d07 INFO Applying 416182 for revision 121566... INFO uploaded 1_416182.patch to https://buildkite.com/organizations/llvm-project/pipelines/premerge-checks/builds/94599/jobs/ee057fd3-a5f6-469f-8ebd-dd087982b0f1/artifacts/b22020f8-e0ae-4eef-a4f7-b5be718dc8ee ERROR exception: Cmd('git') failed due to: exit code(1) cmdline: git push --force origin phab-diff-416182 stderr: 'remote: Internal Server Error To ssh://github.com/llvm-premerge-tests/llvm-project ! [remote rejected] phab-diff-416182 -> phab-diff-416182 (Internal Server Error) error: failed to push some refs to 'ssh://github.com/llvm-premerge-tests/llvm-project'' Reporting results to Phabricator build https://reviews.llvm.org/harbormaster/build/230024 report issue failed 🚨 Error: The command exited with status 1 user command error: exit status 1
Mmmh, indeed. "Internal server error" 🤔
Giving my LGTM assuming you are still doing the Python code cleanup. Please observe the Polly buildbots after committing.
Giving my LGTM assuming you are still doing the Python code cleanup. Please observe the Polly buildbots after committing.
Thanks, will keep an eye for sure.
Can you move all the RUN lines together at the beginning of the file? It is surprising to find another RUN line after e.g. having adapted the others.