Skip to content

[LLVM][Triple] Add an argument to specify canonical form to Triple::normalize #122935

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

Merged
merged 1 commit into from
Jan 15, 2025

Conversation

shiltian
Copy link
Contributor

Currently, the output of Triple::normalize can vary depending on how the
Triple object is constructed, producing a 3-field, 4-field, or even 5-field
string. However, there is no way to control the format of the output, as all
forms are considered canonical according to the LangRef.

This lack of control can be inconvenient when a specific format is required. To
address this, this PR introduces an argument to specify the desired format (3,
4, or 5 identifiers), with the default set to none to maintain the current
behavior. If the requested format requires more components than are available in
the actual Data, "unknown" is appended as needed.

Copy link
Contributor Author

shiltian commented Jan 14, 2025

Copy link
Member

@lenary lenary left a comment

Choose a reason for hiding this comment

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

I'm so confused by this addition

What if I ask for a 4-tuple, and the code produces a 5-tuple? Right now, i think I get a 5-tuple? It's not clear what the code should do here. This seems like an oversight in working out what you intend.

Are you sure that the items in the 4-/5-tuples are in their correct locations? I'm not sure you can just add unknown at the end, but I could be wrong.

I'd like to see a lot more testing with real triples, not totally fake ones. That would probably answer whether the right thing is happening in the latter case with a lot more certainty.

(I'm not sure who the maintainer of this code really is, tbh, but I realise you picked me as I was the last to touch this code - as part of the TargetParser migration. This is reasonable but it means I'm coming at this reasonably "fresh").

@shiltian
Copy link
Contributor Author

What if I ask for a 4-tuple, and the code produces a 5-tuple? Right now, i think I get a 5-tuple?

Yes.

It's not clear what the code should do here. This seems like an oversight in working out what you intend.

I'm not sure what we should do here, but my second thought would be to cut it to the asked form.

Are you sure that the items in the 4-/5-tuples are in their correct locations? I'm not sure you can just add unknown at the end, but I could be wrong.

The code to expand Components is at the end of the function. At that moment all the other identifiers have been at the right location. I think It is safe to expand the array and add unknown here.

I'd like to see a lot more testing with real triples, not totally fake ones. That would probably answer whether the right thing is happening in the latter case with a lot more certainty.

I can definitely do that.

…normalize`

Currently, the output of `Triple::normalize` can vary depending on how the
`Triple` object is constructed, producing a 3-field, 4-field, or even 5-field
string. However, there is no way to control the format of the output, as all
forms are considered canonical according to the LangRef.

This lack of control can be inconvenient when a specific format is required. To
address this, this PR introduces an argument to specify the desired format (3,
4, or 5 identifiers), with the default set to none to maintain the current
behavior. If the requested format requires more components than are available in
the actual `Data`, `"unknown"` is appended as needed.
@shiltian shiltian force-pushed the users/shiltian/normalize-with-num-fields branch from c7c5b74 to bd285ed Compare January 14, 2025 19:17
Copy link
Member

@lenary lenary left a comment

Choose a reason for hiding this comment

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

LGTM. I'm happy given the additional testing.

Copy link
Contributor Author

shiltian commented Jan 15, 2025

Merge activity

  • Jan 14, 8:10 PM EST: A user started a stack merge that includes this pull request via Graphite.
  • Jan 14, 8:12 PM EST: A user merged this pull request with Graphite.

@shiltian shiltian merged commit ebef440 into main Jan 15, 2025
8 checks passed
@shiltian shiltian deleted the users/shiltian/normalize-with-num-fields branch January 15, 2025 01:12
searlmc1 pushed a commit to ROCm/llvm-project that referenced this pull request Mar 28, 2025
…normalize` (llvm#122935)

Currently, the output of `Triple::normalize` can vary depending on how the
`Triple` object is constructed, producing a 3-field, 4-field, or even 5-field
string. However, there is no way to control the format of the output, as all
forms are considered canonical according to the LangRef.

This lack of control can be inconvenient when a specific format is required. To
address this, this PR introduces an argument to specify the desired format (3,
4, or 5 identifiers), with the default set to none to maintain the current
behavior. If the requested format requires more components than are available in
the actual `Data`, `"unknown"` is appended as needed.

(cherry picked from commit ebef440)
rocm-ci pushed a commit to ROCm/llvm-project that referenced this pull request May 20, 2025
…normalize` (llvm#122935)

Currently, the output of `Triple::normalize` can vary depending on how the
`Triple` object is constructed, producing a 3-field, 4-field, or even 5-field
string. However, there is no way to control the format of the output, as all
forms are considered canonical according to the LangRef.

This lack of control can be inconvenient when a specific format is required. To
address this, this PR introduces an argument to specify the desired format (3,
4, or 5 identifiers), with the default set to none to maintain the current
behavior. If the requested format requires more components than are available in
the actual `Data`, `"unknown"` is appended as needed.

(cherry picked from commit ebef440)
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.

2 participants