[SelectionDAG] Folding ZERO-EXTEND/SIGN_EXTEND poison to Poison value in getNode #122741
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-backend-aarch64 @llvm/pr-subscribers-backend-powerpc Author: zhijian lin (diggerlin) Changesin the function ``` |
|
✅ With the latest revision this PR passed the undef deprecator. |
|
Unfortunately, I think the IR semantics do not say that we can do this. For example: The result of sign-extending |
|
The issue that the effect of |
|
I think we would be able to do this for poison, but not undef. Unfortunately SelectionDAG turns poison into undef. |
thanks, I will take a look the pass |
The IR documentation does not appear to support this. It says
and
Any replacement of poison of the narrower type with a value (or I think the direction to take here is to see if the middle-end can change the function parameter to be |
My understanding is that (zext/sext undef) needs to produce a value where all the extended bits are 0 for zext or match the original sign bit for sext. According to InstCombine, (sext/zext poison) can constant fold to poison in the larger type. If SelectionDAG preserved the difference between poison and undef, it could also constant fold to poison. |
It sounds like you have more context than I do then. I believe that the description of |
There's a general statement in the Poison Values section. https://llvm.org/docs/LangRef.html#poison-values "Most instructions return ‘poison’ when one of their arguments is ‘poison’. A notable exception is the select instruction. Propagation of poison can be stopped with the freeze instruction." |
I added some printf into the in the line https://github.com/intel/llvm/blob/sycl/llvm/lib/Transforms/IPO/DeadArgumentElimination.cpp#L336, it converts the unused argument to the patch https://reviews.llvm.org/D125983 change the unused argument from but according to the https://releases.llvm.org/11.0.0/docs/LangRef.html#poison-values there is description as: There is currently no way of representing a poison value in the IR; they only exist when produced by operations such as add with the nsw flag. in the patch https://reviews.llvm.org/D125983, the poison can be represented a argument. does the document need to be modified? |
The "up-to-date" version of the document is here: https://llvm.org/docs/LangRef.html#poison-values. |
diggerlin
force-pushed
the
digger/122728
branch
from
9bd90e7 to
4e80a92
Compare
diggerlin
changed the title
li of 0 into arg registers instruction of unused arguments
diggerlin
changed the title
li of 0 into arg registers instruction of unused arguments
Comment on lines
980
to
981
| // FixMe: If the node represents a poison value, replace it with an undef | ||
| // value. |
There was a problem hiding this comment.
Can you explain what is to be fixed here? The text seems to explain what the code is doing (not what it should do differently).
There was a problem hiding this comment.
Poison is more restrictive than undef. Since we replace poison with undef here, the poison information will be lost after SelectionDAGLegalize::LegalizeLoadOps(SDNode *Node) is called.
If we need to retain the poison information after SelectionDAGLegalize::LegalizeLoadOps(SDNode *Node) in the future, we will need to modify the code accordingly.
| @@ -9237,6 +9263,11 @@ SDValue SelectionDAG::getLoad(ISD::MemIndexedMode AM, ISD::LoadExtType ExtType, | |||
|
|
|||
| SDVTList VTs = Indexed ? | |||
| getVTList(VT, Ptr.getValueType(), MVT::Other) : getVTList(VT, MVT::Other); | |||
|
|
|||
| // FixedMe: lower poison to undef. | |||
There was a problem hiding this comment.
Same comment. Also: I think the convention is FIXME.
arsenm
reviewed
Feb 4, 2025
There was a problem hiding this comment.
Title should be about introducing poison to the DAG, not something about arguments I don't understand
This PR as-is is not about introducing poison to the DAG (that's the how, not the what). The actual operative change is the handling of poison for the width extensions.
Would "Introduce poison to the DAG; address signed/zero extend of poison" work?
These should not be mixed in. Introducing poison should be separate from any of this other stuff |
|
Reproducing @diggerlin's response to #122741 (comment):
|
diggerlin
force-pushed
the
digger/122728
branch
from
5d02652 to
1202242
Compare
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/155/builds/8559 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/35/builds/9384 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/208/builds/538 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/125/builds/7040 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/55/builds/10191 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/202/builds/808 Here is the relevant piece of the build log for the reference |
|
It looks like this broke check-llvm everywhere. I've reverted it for now. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/76/builds/8921 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/143/builds/7135 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/198/builds/3867 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/85/builds/7898 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/24/builds/7600 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/164/builds/9222 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/145/builds/6502 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/94/builds/6413 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/199/builds/2914 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/4/builds/6354 Here is the relevant piece of the build log for the reference |
|
I created a patch #136636 to fix the test cases which cause the buildbot fail |
diggerlin
added a commit
to diggerlin/llvm-project
that referenced
this pull request
Apr 22, 2025
… in getNode (llvm#122741) The PR will fix the issue llvm#122728 This patch addresses the signed/zero extension of poison by using a poison value of the extended type instead of a constant zero of the extended type.
llvm-sync bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
May 6, 2025
…son to Poison value in getNode (#122741)" This reverts commit f12078e. Breaks `check-llvm`, see comments on llvm/llvm-project#122741
IanWood1
pushed a commit
to IanWood1/llvm-project
that referenced
this pull request
May 6, 2025
… in getNode (llvm#122741) The PR will fix the issue llvm#122728 This patch addresses the signed/zero extension of poison by using a poison value of the extended type instead of a constant zero of the extended type.
IanWood1
pushed a commit
to IanWood1/llvm-project
that referenced
this pull request
May 6, 2025
…on value in getNode (llvm#122741)" This reverts commit f12078e. Breaks `check-llvm`, see comments on llvm#122741
IanWood1
pushed a commit
to IanWood1/llvm-project
that referenced
this pull request
May 6, 2025
… in getNode (llvm#122741) The PR will fix the issue llvm#122728 This patch addresses the signed/zero extension of poison by using a poison value of the extended type instead of a constant zero of the extended type.
IanWood1
pushed a commit
to IanWood1/llvm-project
that referenced
this pull request
May 6, 2025
…on value in getNode (llvm#122741)" This reverts commit f12078e. Breaks `check-llvm`, see comments on llvm#122741
IanWood1
pushed a commit
to IanWood1/llvm-project
that referenced
this pull request
May 6, 2025
… in getNode (llvm#122741) The PR will fix the issue llvm#122728 This patch addresses the signed/zero extension of poison by using a poison value of the extended type instead of a constant zero of the extended type.
IanWood1
pushed a commit
to IanWood1/llvm-project
that referenced
this pull request
May 6, 2025
…on value in getNode (llvm#122741)" This reverts commit f12078e. Breaks `check-llvm`, see comments on llvm#122741
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The PR will fix the issue #122728
This patch addresses the signed/zero extension of poison by using a poison value of the extended type instead of a constant zero of the extended type.