[SelectionDAG] Add pcsections recursively on SDNode values

[martin-fink]

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.

[melver]

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'

[melver]

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

[arsenm]

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]

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]

[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]

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]

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

[martin-fink]

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

Use recursive function instead of lambda

[martin-fink]

Remove unneeded import

[martin-fink]

Rebase main

[melver]

Nice!

Minor comments below. Otherwise LGTM.

llvm/include/llvm/CodeGen/SelectionDAG.h

2284–2298

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

2378

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.

2378

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]

llvm/include/llvm/CodeGen/SelectionDAG.h

2378

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

[martin-fink]

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

• Rename addPcsectionsr
• Rename Once to Visited
• Move function out of header file

[melver]

Thanks!
I'll land this for you.

[melver]

llvm/include/llvm/CodeGen/SelectionDAG.h

2379

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