Page MenuHomePhabricator

Load pass plugins during option processing, so that plugin options are registered and live.
ClosedPublic

Authored by w2yehia on Mar 13 2022, 9:53 PM.

Diff Detail

Event Timeline

w2yehia created this revision.Mar 13 2022, 9:53 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 13 2022, 9:53 PM
w2yehia requested review of this revision.Mar 13 2022, 9:53 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 13 2022, 9:53 PM
mehdi_amini added inline comments.Mar 14 2022, 10:58 AM
llvm/tools/opt/opt.cpp
602

Should we sanitize that the new PM is enabled if PassPlugins is provided and error otherwise?

w2yehia updated this revision to Diff 415310.Mar 14 2022, 9:37 PM

Issue an error when -load-pass-plugin is used with legacy PM.

w2yehia marked an inline comment as done.Mar 14 2022, 9:38 PM
w2yehia added inline comments.
llvm/tools/opt/opt.cpp
602

Thanks. Done.

mehdi_amini accepted this revision.Mar 14 2022, 9:41 PM
This revision is now accepted and ready to land.Mar 14 2022, 9:41 PM
w2yehia closed this revision.Mar 15 2022, 5:46 AM
w2yehia marked an inline comment as 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?

w2yehia reopened this revision.Mar 16 2022, 11:44 AM
This revision is now accepted and ready to land.Mar 16 2022, 11:44 AM

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.

w2yehia updated this revision to Diff 416182.EditedMar 17 2022, 8:20 AM

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.

w2yehia marked an inline comment as done.Mar 17 2022, 8:21 AM

Could you rebase the patch? The premerge-check's patch application failed: https://buildkite.com/llvm-project/diff-checks/builds/94599

Meinersbur added inline comments.Mar 17 2022, 9:23 AM
polly/test/lit.site.cfg.in
29–33

Parens are unnecessary here

49–52

Preprending empty string does nothing

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
Meinersbur accepted this revision.Mar 17 2022, 12:20 PM

Mmmh, indeed. "Internal server error" 🤔

Giving my LGTM assuming you are still doing the Python code cleanup. Please observe the Polly buildbots after committing.

This revision was landed with ongoing or failed builds.Mar 17 2022, 8:28 PM
This revision was automatically updated to reflect the committed changes.

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.