-
Notifications
You must be signed in to change notification settings - Fork 86
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[aievec] to-llvm flow for aievec.ext op with I128.I512 extract intrinsic #1490
Merged
jamestcl-amd
merged 7 commits into
Xilinx:main
from
jamestcl-amd:aievec_ext_llvm_128_512
May 17, 2024
Merged
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
b70dc12
Correct the I128.I512 ext intrinsic in xllvm
jamestcl-amd f7bcd50
update xllvm to external translation test
jamestcl-amd 48977e4
Add conversion pattern for I128.I512 ext intrinsic op
jamestcl-amd c2ca3dd
Add lit test converage for the conversion pattern
jamestcl-amd ca17f59
Adding explicit clarification of the semantic of aievec.ext/shift ops
jamestcl-amd a30f0e5
Improve the lowering pattern
jamestcl-amd 5cefa6a
Merge branch 'main' into aievec_ext_llvm_128_512
jamestcl-amd File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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
This file contains 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
This file contains 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
This file contains 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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this correct? If I understand correctly, the process to extract a subvector from a given position is to shift the whole vector by the number of bits up to the position, and then extracting the lowest part. For
16-bit
element vectors and an extraction from position 3, that's3 x 16 = 48 bit
, but for32-bit
element vectors, shouldn't it be3 x 32 = 96 bit
? Am I misunderstanding something?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
shift
amount for theshift
op/intrinsic is in the number of bytes. TheI128.I512
extract intrinsic is special. It always extracts the lowest 128-bit vector from the source 512-bit vector. Forindex=0
vector extraction, we actually don't need any vector shift beforehand, so I have updated the conversion pattern for this scenario. As forindex=3
vector extraction, the shift amount is48 bytes = 384 bits = 3*128 bits
. After the right shift of 48 bytes, theI128.I512
intrinsic extracts the lowest 128-bit vector, which is correct. I have updated theaievec.ext/shift
op descriptions, to explicitly clarify the shift amount in bytes and the extraction index to be either 0--1 or 0--3. Let me know if there is anything else I can do :).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aaaah! Got it, I was trying to make the maths work in my head and failing, thanks for the clarification 🙂