Skip to content
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

hexagon: adapt test for upstream output changes #97838

Merged
merged 1 commit into from
Jun 9, 2022

Conversation

durin42
Copy link
Contributor

@durin42 durin42 commented Jun 7, 2022

The output of IR formatting changed slightly in upstream rev
a0bc67e
(https://reviews.llvm.org/D123096). I'm not actually sure what any of
that means, as I don't even know what hexagon is in this context, but
this change allows the test to pass on both old and new LLVMs.

r? @nikic

The output of IR formatting changed slightly in upstream rev
a0bc67e
(https://reviews.llvm.org/D123096). I'm not actually sure what any of
that means, as I don't even know what hexagon is in this context, but
this change allows the test to pass on both old and new LLVMs.

r? @nikic
@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikic (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 7, 2022
@nikic
Copy link
Contributor

nikic commented Jun 7, 2022

Not familiar with Hexagon assembly and a bit unsure about these changes. Something like r0 = ##extern_static gets printed now, where the first # comes from the inline assembly string and the second is inserted by LLVM. Did the responsibility for adding the # shift here, and it should be dropped from the inline assembly string?

cc @Amanieu @androm3da

@durin42
Copy link
Contributor Author

durin42 commented Jun 7, 2022

I have no idea. Realistically I can probably ignore this test failure for us (we don't care about hexagon) but I figured we should try and fix things as quickly as possible. Maybe it's a regression upstream in some printing?

@nikic
Copy link
Contributor

nikic commented Jun 7, 2022

From https://developer.qualcomm.com/qfile/29900/80-n2040-8_h_programmers_ref_v5.pdf it looks like ## is at least valid and seems to have something to do with whether something is a 32-bit value.

Copy link
Contributor

@androm3da androm3da left a comment

Choose a reason for hiding this comment

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

Not familiar with Hexagon assembly and a bit unsure about these changes. Something like r0 = ##extern_static gets printed now, where the first # comes from the inline assembly string and the second is inserted by LLVM. Did the responsibility for adding the # shift here, and it should be dropped from the inline assembly string?

cc @Amanieu @androm3da

LGTM but I agree I would like to understand the nature of this change. I'll investigate.

@@ -73,7 +73,7 @@ macro_rules! check_reg {

// CHECK-LABEL: sym_static:
// CHECK: InlineAsm Start
// CHECK: r0 = #extern_static
// CHECK: r0 = {{#+}}extern_static
Copy link
Contributor

Choose a reason for hiding this comment

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

The # versus ## syntax refers to whether the assembler should force an immediate-extender word prior to this word in the encoding.

This test change is appropriate, but I am curious about its origin so I will investigate. Additional extender words can result in increased code size. This test isn't intended to focus on this aspect of the emitted instructions, so I think it's appropriate as-is.

Copy link
Member

Choose a reason for hiding this comment

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

Could it be that the assembler is reserving space for a full-size immediate that will be filled in by the linker later?

@@ -108,7 +108,7 @@ pub unsafe fn sym_fn() {
// CHECK: InlineAsm Start
// CHECK: {
// CHECK: r{{[0-9]+}} = r0
// CHECK: memw(r1) = r{{[0-9]+}}
// CHECK: memw(r1{{(\+#0)?}}) = r{{[0-9]+}}
Copy link
Contributor

Choose a reason for hiding this comment

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

This offset is implied in the baseline and it would appear that the code emitted may have changed slightly.

This test change is appropriate.

@Amanieu
Copy link
Member

Amanieu commented Jun 8, 2022

Note that the output being checked here is disassembly generated by LLVM, not the actual code that is passed as input to the assembler. So it's fine to have slight variations in the disassembly output.

@bors r+

@bors
Copy link
Contributor

bors commented Jun 8, 2022

📌 Commit d92e213 has been approved by Amanieu

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 8, 2022
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Jun 9, 2022
hexagon: adapt test for upstream output changes

The output of IR formatting changed slightly in upstream rev
a0bc67e
(https://reviews.llvm.org/D123096). I'm not actually sure what any of
that means, as I don't even know what hexagon is in this context, but
this change allows the test to pass on both old and new LLVMs.

r? `@nikic`
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 9, 2022
Rollup of 5 pull requests

Successful merges:

 - rust-lang#95632 (impl Read and Write for VecDeque<u8>)
 - rust-lang#95860 (Stabilize `$$` in Rust 1.63.0)
 - rust-lang#97838 (hexagon: adapt test for upstream output changes)
 - rust-lang#97843 (Relax mipsel-sony-psp's linker script)
 - rust-lang#97874 (rewrite combine doc comment)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit fa4f8f1 into rust-lang:master Jun 9, 2022
@rustbot rustbot added this to the 1.63.0 milestone Jun 9, 2022
@durin42 durin42 deleted the llvm-15-hexagon branch May 11, 2023 09:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants