This is an archive of the discontinued LLVM Phabricator instance.

[SelectionDAG] Give all the target specific subclasses of SelectionDAGISel their own pass ID.
ClosedPublic

Authored by craig.topper on Dec 15 2022, 1:01 PM.

Details

Summary

Previously we had a shared ID in SelectionDAGISel. AMDGPU has an
initializePass function for its subclass of SelectionDAGISel. No
other target does.

This causes all target specific SelectionDAGISel passes to be known
as "amdgpu-isel".

I'm not sure what would happen if another target tried to implement
an initializePass function too since the ID is already claimed.

This patch gives all targets their own ID and passes it down to
SelectionDAGISel constructor to MachineFunctionPass's constructor.

Unfortunately, I think this causes most targets to lose
print-before/after-all support for their SelectionDAGISel pass.
And they probably no longer support start/stop-before/after. We
can add initializePass functions to fix this as a follow up. NOTE:
This was probably also broken if the AMDGPU target isn't compiled in.

Step 1 to fixing PR59538.

Diff Detail

Event Timeline

craig.topper created this revision.Dec 15 2022, 1:01 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 15 2022, 1:01 PM
craig.topper requested review of this revision.Dec 15 2022, 1:01 PM

Thanks for the (quick) patch!

Unfortunately, I think this causes most targets to lose
print-before/after-all support for their SelectionDAGISel pass.

I now see:

# *** IR Dump After Finalize ISel and expand pseudo-instructions (finalize-isel) ***:

for llc -mtriple=x86_64-linux-gnu /tmp/x.ll -stop-after=finalize-isel -o - -print-before-all
(good) and -mtriple=aarch64-linux-gnu (good). So perhaps that can perhaps be dropped from the commit message. Or was there another invocation of llc where you observed this?

And they probably no longer support start/stop-before/after.

Let's check that too!

When I apply this on top of my stack o' patches: https://reviews.llvm.org/D140160, my tests running -start-before=amdgpu-isel both fail.

LLVM ERROR: Cannot stop compilation after pass that is not run

This is expected (good).

Now let's see if I can change -start-before=amdgpu-isel to -start-before=finalize-isel...
and test's fail with no output. So indeed seems like we'll need that part 2.

Or was there another invocation of llc where you observed this?

Doesn't -print-after-all only print if changes were made (given for finalize-isel)? Rerunning my above tests with -print-after-all rather than -print-before-all I still see finalize-isel. I think this confirms that D140161 does not break -print-after-all/-print-before-all.

(Though one curiosity I do see for llc -mtriple=aarch64-linux-gnu /tmp/x.ll -stop-after=finalize-isel -o - -print-after-all is that MIR is printed after aarch64-local-dynamic-tls-cleanup which runs before finalize-isel; WAT?!)

Doesn't -print-after-all only print if changes were made (given for finalize-isel)?

I don't think it checks that.

llvm/lib/Target/AVR/AVRISelDAGToDAG.cpp
24

Random observation. This is curiously in the llvm namespace instead of an anonymous namespace.

Doesn't -print-after-all only print if changes were made (given for finalize-isel)?

I don't think it checks that.

There's -print-changed and -print-before-changed.

Not allowing -stop-before/-stop-after the actual isel pass is probably a feature, not a bug. We don't run the verifier between that and finalizeisel. You can see verifier failing MIR at that point which would confuse

Not allowing -stop-before/-stop-after the actual isel pass is probably a feature, not a bug. We don't run the verifier between that and finalizeisel. You can see verifier failing MIR at that point which would confuse

Without -stop-after, how are folks expected to test SelectionDAG in isolation?

(-stop-before is the other side of the same coin as -stop-after for whatever pass comes before finalize-isel. It's asymmetric to support one without the other, but if I can only have one, that's good enough to do my job).

Not allowing -stop-before/-stop-after the actual isel pass is probably a feature, not a bug. We don't run the verifier between that and finalizeisel. You can see verifier failing MIR at that point which would confuse

Without -stop-after, how are folks expected to test SelectionDAG in isolation?

You kind of don't. That's one of the major issues with it. After finalizeisel is the first point where you're consistently guaranteed verifiable mir

Not allowing -stop-before/-stop-after the actual isel pass is probably a feature, not a bug. We don't run the verifier between that and finalizeisel. You can see verifier failing MIR at that point which would confuse

Without -stop-after, how are folks expected to test SelectionDAG in isolation?

You kind of don't. That's one of the major issues with it. After finalizeisel is the first point where you're consistently guaranteed verifiable mir

So why isn't -stop-after=finalizeisel working "not a bug?" (Perhaps I should have s/SelectionDAG/SelectionDAGISel/ above)

So why isn't -stop-after=finalizeisel working "not a bug?" (Perhaps I should have s/SelectionDAG/SelectionDAGISel/ above)

-stop-after=finalize-isel should work. That's what I always use

arsenm accepted this revision.Dec 15 2022, 3:29 PM

So why isn't -stop-after=finalizeisel working "not a bug?" (Perhaps I should have s/SelectionDAG/SelectionDAGISel/ above)

-stop-after=finalize-isel should work. That's what I always use

Nothing also stops you from using -stop-before=finalize-isel, which is effectively the same thing

This revision is now accepted and ready to land.Dec 15 2022, 3:29 PM
This revision was landed with ongoing or failed builds.Dec 15 2022, 3:49 PM
This revision was automatically updated to reflect the committed changes.