Page MenuHomePhabricator

[SelectionDAG] Add pcsections recursively on SDNode values
ClosedPublic

Authored by martin-fink on Jan 5 2023, 6:12 AM.

Details

Summary

When adding pcsetions to SDNodes, recursively add them to all values of
the node as well.

[SelectionDAG] Add missing setValue calls in visitIntrinsicCall

This commit introduces missing setValue calls in SelectionDAGBuilder for
mem-transfer intrinsic calls. These setValue calls are required in order to
propagate pcsections metadata from IR to MIR.

Diff Detail

Event Timeline

martin-fink created this revision.Jan 5 2023, 6:12 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 5 2023, 6:12 AM
martin-fink requested review of this revision.Jan 5 2023, 6:12 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 5 2023, 6:12 AM
melver requested changes to this revision.Jan 5 2023, 6:38 AM
melver added inline comments.
llvm/test/CodeGen/AArch64/SelectionDAG/arm64-pcsections-selectiondag.ll
1 ↗(On Diff #486543)

Similar to x86 comment:

I think we want to say -global-isel=0, to ensure GlobalISel isn't used?

Also, just call it similar to the x86 one 'pcsections-memtransfer.ll', i.e. it'll end up in 'CodeGen/AArch64/pcsections-memtransfer.ll'.

87–100 ↗(On Diff #486543)

I see no pcsections, is it still missing?

109–122 ↗(On Diff #486543)

I see no pcsections, is it still missing?

131–144 ↗(On Diff #486543)

I see no pcsections, is it still missing?

llvm/test/CodeGen/X86/SelectionDAG/x64-pcsections-memtransfer.ll
1 ↗(On Diff #486543)

I think creating a SelectionDAG subdir is unnecessary (you're the first to create it). I think we can just dump it in X86/

Also, I think we need to explicitly make sure global-isel isn't used as it might be enabled in future, at which point it doesn't test SelectionDAG anymore.

E.g. see llvm/test/CodeGen/X86/switch-bit-test-unreachable-default.ll

I'd name this test simply 'pcsections-memtransfer.ll'

This revision now requires changes to proceed.Jan 5 2023, 6:38 AM
melver added a comment.Jan 5 2023, 6:39 AM

Also update commit message to say something about "mem transfer" intrinsics.

arsenm added a subscriber: arsenm.Jan 5 2023, 6:47 AM
arsenm added inline comments.
llvm/test/CodeGen/AArch64/SelectionDAG/arm64-pcsections-selectiondag.ll
1 ↗(On Diff #486543)

Just because it's a test for sdag doesn't mean it also shouldn't pass for globalisel. I wouldn't explicitly turn it off

martin-fink added inline comments.Jan 5 2023, 6:56 AM
llvm/test/CodeGen/AArch64/SelectionDAG/arm64-pcsections-selectiondag.ll
87–100 ↗(On Diff #486543)

I think I've overlooked that pcsections are not passed to MIR for the .element.unordered.atomic instrinsics on AArch64.
As the issue for that probably lies somewhere else and not in the scope of this commit (the pcsections are passed to MIR on X86, for example), I would suggest that I (for now) remove these three functions from the test in this commit and have a look at them later on.
What do you think about that?

martin-fink edited the summary of this revision. (Show Details)

[SelectionDAG] Add missing setValue calls in visitIntrinsicCall

This commit introduces missing setValue calls in SelectionDAGBuilder for
mem-transfer intrinsic calls. These setValue calls are required in order to
propagate pcsections metadata from IR to MIR.

melver added inline comments.Jan 5 2023, 11:11 AM
llvm/test/CodeGen/AArch64/SelectionDAG/arm64-pcsections-selectiondag.ll
87–100 ↗(On Diff #486543)

I think it's somewhere in the AArch64-specific bits of SelectionDAG.

I think if you want this working properly, it should be fixed.

Otherwise, the support for the mem-transfer functions seems incomplete and whatever you intend to use !pcsections for, will be half working only.

martin-fink retitled this revision from [SelectionDAG] Add missing setValue calls in visitIntrinsicCall to [SelectionDAG] Add pcsections recursively on SDNode values.
martin-fink edited the summary of this revision. (Show Details)

Recursively set pcsections on SDNodes to make sure pcsections are propagated to all generated instructions.

Updating D141048: [SelectionDAG] Add pcsections recursively on SDNode values

Use recursive function instead of lambda

Remove unneeded import

martin-fink edited the summary of this revision. (Show Details)

Rebase main

Nice!

Minor comments below. Otherwise LGTM.

llvm/include/llvm/CodeGen/SelectionDAG.h
2266–2267

This looks like DFS - this is typically called the visited set. So I'd just call it "Visited".

2346

No need for different name, just "addPCSections" - the fact it has different arguments means it's overloaded and there's no conflict.

Given it's private and has documentation above it, it seems cleaner.

2346

This is a rather large non-trivial function now, I'd now define it in the header, but move it to the .cpp file (like most functions here).

melver added inline comments.Wed, Jan 25, 3:30 AM
llvm/include/llvm/CodeGen/SelectionDAG.h
2346

... I'd _not_ define it in the header ...

martin-fink edited the summary of this revision. (Show Details)

Made the following changes based on @melver's comments:

  • Rename addPcsectionsr
  • Rename Once to Visited
  • Move function out of header file
melver accepted this revision.Thu, Jan 26, 6:23 AM

Thanks!
I'll land this for you.

This revision is now accepted and ready to land.Thu, Jan 26, 6:23 AM
melver added inline comments.Thu, Jan 26, 6:39 AM
llvm/include/llvm/CodeGen/SelectionDAG.h
2347

I think this function wants to receive a reference to the set - I'll fix it up for you.