Skip to content

Conversation

@s-barannikov
Copy link
Contributor

@s-barannikov s-barannikov commented Dec 12, 2024

RFC

Landed changes:
#138381 RISCV
#138450 Sparc
#138869 XCore
#138874 ARC, CSKY, Lanai
#138878 MSP430
#139407 AVR
#139449 M68k
#140472 AArch64
#166253 Xtensa
#166259 WebAssembly
#166499 BPF
#168120 VE

@github-actions

This comment was marked as off-topic.

@llvm llvm deleted a comment from github-actions bot Dec 12, 2024
@s-barannikov s-barannikov force-pushed the tablegen/sdnode-info-emitter branch from 2a57488 to b90b16f Compare December 12, 2024 16:44
@github-actions

This comment was marked as off-topic.

/// Test if this node has a target-specific
/// memory-referencing opcode (in the \<target\>ISD namespace and
/// greater than FIRST_TARGET_MEMORY_OPCODE).
bool isTargetMemoryOpcode() const {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we still need isTargetMemoryOpcode(). It doesn't have any callers.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It has one use in the assert in SelectionDAG::getMemIntrinsicNode.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops. I was looking in my local repo and should have checked for FIRST_TARGET_MEMORY_OPCODE to see that assert

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If nothing else is checking it, does that assert provide any value? Can we just allow any target opcode to be a memory opcode?

Copy link
Contributor Author

@s-barannikov s-barannikov Dec 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I remembered it has one more "use". Before this patch it is implicitly checked here. isTargetStrictFPOpcode returns true for memory opcodes as well. mayRaiseFPException is then used here to decide whether the resulting MachineInstr needs NoFPExcept flag. IIUC it will not set this flag on MachineInstrs if the matched SDNode(s) were flagged as memory accessing (or having strict-fp semantics).

This behavior was introduced in 6333679. I couldn't conclude if it is safe to return false for memory opcodes so I left it as is.

After this patch the check is explicit, please see SelectionDAGTargetInfo.cpp:mayRaiseFPException.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ran CodeGen tests with the memory opcode check disabled and there was one failure in
X86/fp-intrinsics-flags.ll. The following diff resolves it:

image

There was a discussion in https://reviews.llvm.org/D72466 that can be related to this. Can you confirm that this is a safe change? If so, then quoting @uweigand from https://reviews.llvm.org/D71841 I assume the check for memory opcode can be dropped:

Note that there a bit of a quirk in identifying target nodes that are both memory nodes and strict FP nodes. To simplify the logic, right now all target memory nodes are automatically also considered strict FP nodes -- this could be fixed by adding one more range.

@s-barannikov s-barannikov marked this pull request as ready for review December 13, 2024 13:37
@s-barannikov s-barannikov force-pushed the tablegen/sdnode-info-emitter branch from b90b16f to ad38786 Compare December 14, 2024 12:34
s-barannikov added a commit to s-barannikov/llvm-project that referenced this pull request Dec 14, 2024
A future patch adds a couple of new methods to this class,
which will need to be overridden by these targets.

Part of llvm#119709.
s-barannikov added a commit that referenced this pull request Dec 16, 2024
…argets (#119968)

#119969 adds a couple of new methods to this class, which will need to
be overridden by these targets.

Part of #119709.

Pull Request: #119968
@s-barannikov s-barannikov force-pushed the tablegen/sdnode-info-emitter branch from ad38786 to 7ed14ef Compare December 16, 2024 20:32
s-barannikov added a commit that referenced this pull request Dec 21, 2024
…de (#119969)

With this change, targets are no longer required to put memory / strict-fp opcodes after special
`ISD::FIRST_TARGET_MEMORY_OPCODE`/`ISD::FIRST_TARGET_STRICTFP_OPCODE` markers.
This will also allow autogenerating `isTargetMemoryOpcode`/`isTargetStrictFPOpcode (#119709).

Pull Request: #119969
@s-barannikov s-barannikov force-pushed the tablegen/sdnode-info-emitter branch 3 times, most recently from 7dd3721 to c315879 Compare December 21, 2024 15:10
@s-barannikov s-barannikov force-pushed the tablegen/sdnode-info-emitter branch 2 times, most recently from 5d1058f to 3ba779a Compare January 14, 2025 18:44
s-barannikov added a commit to s-barannikov/llvm-project that referenced this pull request Jan 15, 2025
This patch adds a simplistic backend that gathers all target-specific
SelectionDAG nodes and emits descriptions for most of them.

This includes generating node enumeration, node names, and information
about node "prototype" that can be used to verify that a node is valid.

The patch also extends SDNode by adding target-specific flags, which
are also included in the generated tables.

Part of llvm#119709, [RFC](https://discourse.llvm.org/t/rfc-tablegen-erating-sdnode-descriptions/83627).
s-barannikov added a commit to s-barannikov/llvm-project that referenced this pull request Jan 15, 2025
This patch adds a simplistic backend that gathers all target-specific
SelectionDAG nodes and emits descriptions for most of them.

This includes generating node enumeration, node names, and information
about node "prototype" that can be used to verify that a node is valid.

The patch also extends SDNode by adding target-specific flags, which
are also included in the generated tables.

Part of llvm#119709, [RFC](https://discourse.llvm.org/t/rfc-tablegen-erating-sdnode-descriptions/83627).
s-barannikov added a commit that referenced this pull request Jan 22, 2025
This patch adds a simplistic backend that gathers all target-specific
SelectionDAG nodes and emits descriptions for most of them.

This includes generating node enumeration, node names, and information
about node "prototype" that can be used to verify that a node is valid.

The patch also extends SDNode by adding target-specific flags, which are
also included in the generated tables.

Part of #119709,
[RFC](https://discourse.llvm.org/t/rfc-tablegen-erating-sdnode-descriptions/83627).

Pull Request: #123002
@s-barannikov s-barannikov force-pushed the tablegen/sdnode-info-emitter branch from 3ba779a to 4c7893b Compare January 22, 2025 06:20
@s-barannikov s-barannikov force-pushed the tablegen/sdnode-info-emitter branch from 4c7893b to ae89705 Compare February 1, 2025 19:08
s-barannikov added a commit to s-barannikov/llvm-project that referenced this pull request Feb 1, 2025
This patch introduces SelectionDAGGenTargetInfo and SDNodeInfo classes,
which provide methods for accessing the generated SDNode descriptions.

RFC: https://discourse.llvm.org/t/rfc-tablegen-erating-sdnode-descriptions
Draft PR: llvm#119709
s-barannikov added a commit to s-barannikov/llvm-project that referenced this pull request Nov 3, 2025
s-barannikov added a commit to s-barannikov/llvm-project that referenced this pull request Nov 3, 2025
This allows SDNodes to be validated against their expected type
profiles and reduces the number of changes required to add a new node.

CALL and RET_CALL do not have a description in td files, and it is not
currently possible to add one as these nodes have both variable operands
and variable results.

Part of llvm#119709.
s-barannikov added a commit that referenced this pull request Nov 5, 2025
This allows SDNodes to be validated against their expected type profiles
and reduces the number of changes required to add a new node.

CALL and RET_CALL do not have a description in td files, and it is not
currently possible to add one as these nodes have both variable operands
and variable results.

This also fixes a subtle bug detected by the enabled verification
functionality. `LOCAL_GET` is declared with `SDNPHasChain` property, and
thus should have both a chain operand and a chain result. The original
code created a node without a chain result, which caused a check in
`SDNodeInfo::verifyNode()` to fail.

Part of #119709.

Pull Request: #166259
s-barannikov added a commit to s-barannikov/llvm-project that referenced this pull request Nov 5, 2025
This allows SDNodes to be validated against their expected type profiles
and reduces the number of changes required to add a new node.

Fix BR_CC/MEMCPY descriptions to match C++ code that creates the nodes.

Part of llvm#119709.
yonghong-song pushed a commit that referenced this pull request Nov 6, 2025
This allows SDNodes to be validated against their expected type profiles
and reduces the number of changes required to add a new node.

Fix BR_CC/MEMCPY descriptions to match C++ code that creates the nodes
(an error detected by the enabled verification functionality).

Also remove redundant `SDNPOutGlue` on `BPFISD::MEMCPY`.

Part of #119709.
@s-barannikov s-barannikov force-pushed the tablegen/sdnode-info-emitter branch from 02a9c19 to b0de7ad Compare November 7, 2025 03:23
vinay-deshmukh pushed a commit to vinay-deshmukh/llvm-project that referenced this pull request Nov 8, 2025
This allows SDNodes to be validated against their expected type profiles
and reduces the number of changes required to add a new node.

Fix BR_CC/MEMCPY descriptions to match C++ code that creates the nodes
(an error detected by the enabled verification functionality).

Also remove redundant `SDNPOutGlue` on `BPFISD::MEMCPY`.

Part of llvm#119709.
s-barannikov added a commit to s-barannikov/llvm-project that referenced this pull request Nov 11, 2025
s-barannikov added a commit that referenced this pull request Nov 14, 2025
@s-barannikov s-barannikov force-pushed the tablegen/sdnode-info-emitter branch from b0de7ad to 9f09e1f Compare November 14, 2025 18:54
s-barannikov added a commit that referenced this pull request Nov 14, 2025
This allows SDNodes to be validated against their expected type profiles
and reduces the number of changes required to add a new node.

There is a couple of nodes that are missing description and one node
that fails validation.

Part of #119709.

Pull Request: #168120
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants