hexagon: adapt test for upstream output changes #97838
Conversation
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
|
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
added
the
S-waiting-on-review
|
Not familiar with Hexagon assembly and a bit unsure about these changes. Something like |
|
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? |
|
From https://developer.qualcomm.com/qfile/29900/80-n2040-8_h_programmers_ref_v5.pdf it looks like |
There was a problem hiding this comment.
Not familiar with Hexagon assembly and a bit unsure about these changes. Something like
r0 = ##extern_staticgets 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?
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 | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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] }} | |||
There was a problem hiding this comment.
This offset is implied in the baseline and it would appear that the code emitted may have changed slightly.
This test change is appropriate.
|
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 |
|
📌 Commit d92e213 has been approved by |
bors
added
S-waiting-on-bors
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`
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
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