Non-standard assembler usage #105

jeremybennett · 2020-04-22T08:26:34Z

The Clang/LLVM compiler is stricter than GCC in complying with the RISC-V assembler specification. If I used the latest Clang/LLVM compiler to compile the benchmarks, I get a failure. Extracting an individual test shows this:

riscv32-unknown-elf-clang --target=riscv32-unknown-elf -march=rv32i -c
-o I-ENDIANESS-01.o -I ../../../riscv-test-env -I
../../../riscv-test-env/p -I ../../../riscv-target/ri5cy I-ENDIANESS-01.S

../../../riscv-target/ri5cy/compliance_io.h:187:12: error: operand must
be a bare symbol name
    la t3, 0x10000000; sw a0, (0)(t3);;

This instruction sequence comes from the expansion of one of the standard header macros.

The error is that the RISC-V standard requires the second operand to the la pseudo-instruction to be a symbol, not a constant. It just happens that GCC accepts a constant - arguably a bug in GCC, certainly an unofficial extension.

It seems to me that a compliance test suite should strictly follow the standard!

(this would also seem to indicate that no one is using Clang/LLVM with the compliance tests, which would certainly seem to be a good sanity check).

The text was updated successfully, but these errors were encountered:

aswaterman · 2020-04-22T08:50:13Z

Historically, the behavior of the GNU assembler has been the de facto spec, though I realize that's a bit hegemonic.

Philosophically, a constant value is a valid address. My preference in this case is to maximize compatibility: update the spec to clarify that constant-valued addresses are legal addresses for the purposes of la, and encourage the LLVM team to follow suit.

neelgala · 2020-04-22T08:50:23Z

Hi Jeremy,

The erro is in the file: compliance_io.h that is managed and provided by the target. Since the compilation, linking and execution of the test is responsibility of the target, the framework cannot impose any restrictions there. If ris5cy wants the compile the tests with clang and gcc then it should change the respective compliance_io.h.

I did a grep and quick glance indicates that atleast this particular case isn't occurring anywhere else in the tests.

jeremybennett · 2020-04-22T09:05:45Z

@aswaterman Agreed the spec needs changing, since it says "symbol". Clang is clear what is a symbol and what is a constant. Initial implementations inherently define specifications - that is common in many areas. We just need to clean up the gaps, so the spec becomes the golden reference.

@neelgala It is reasonable for the compliance test suite to require compliance with the spec, but where bugs are found, responsibility for resolution needs to be placed on the target owner. So this issue should be assigned to that person. Do you know who owns the RI5CY target?

neelgala · 2020-04-22T09:23:53Z

I think @bluewww was the author of the PR for adding ri5scy.

I just wanted to say that it is a bug only if RI5CY wants to be Clang compatible. The compliace framework declares a combination of SDK and Device Target as RISC-V compliant.

Also, I think #107 and #106 (post resolution) should probably end up as guidelines within the Test-format spec.

Maybe the RI5CY guys could fix this.

bluewww · 2020-04-22T13:22:31Z

I track it for now, but I don't really use clang. The quick fix would probably be to define a symbol with the correct constant and load that.

allenjbaum · 2020-04-22T19:20:25Z

The code in question doesn't actually use the constant directly, it does this: #define RVTEST_CUSTOM1 0x10000000 And then uses RVTEST_CUSTOM1 as an argument to a macro. So, is the fix simple to change it to: RVTEST_CUSTOM1 equ 0x10000000. ?

…

On Wed, Apr 22, 2020 at 6:25 AM bluew ***@***.***> wrote: I track it for now, but I don't really use clang. The quick fix would probably be to define a symbol with the correct constant and load that. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#105 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHPXVJRPAFAVDLX4ODCXIG3RN3V4ZANCNFSM4MN6OXTA> .

allenjbaum · 2020-04-29T22:06:39Z

The other fix (which I've mentioned elsewhere) is to change the LA in riscv-target/ri5cy/compliance_io.h to be LI.

sequencer · 2020-11-30T01:11:14Z

Same issue in compiling riscv-tests with

clang -static -mcmodel=medany -nostdlib -nostartfiles -mno-relax --target=riscv64 -mcpu=rocket-rv64 -fuse-ld=lld -mabi=lp64 -march=rv64g -fvisibility=hidden -Ienv/v -Iisa/macros/scalar -I/usr/riscv64-elf/include -L/usr/riscv64-elf/lib env/v/entry.S -DENTROPY=0 env/v/*.c isa/rv64si/sbreak.S

clang is not happy with symbol defined in ld script.

env/v/entry.S:65:10: error: operand must be a bare symbol name
  la sp, (_end + 4096) - ((64 / 8) * 36)

MaskRay · 2020-11-30T03:24:22Z

Same issue in compiling riscv-tests with

clang -static -mcmodel=medany -nostdlib -nostartfiles -mno-relax --target=riscv64 -mcpu=rocket-rv64 -fuse-ld=lld -mabi=lp64 -march=rv64g -fvisibility=hidden -Ienv/v -Iisa/macros/scalar -I/usr/riscv64-elf/include -L/usr/riscv64-elf/lib env/v/entry.S -DENTROPY=0 env/v/*.c isa/rv64si/sbreak.S

clang is not happy with symbol defined in ld script.

env/v/entry.S:65:10: error: operand must be a bare symbol name
  la sp, (_end + 4096) - ((64 / 8) * 36)

I have a pending LLVM MC patch to fix this. You can use la sp, _end + 4096 - ((64 / 8) * 36) for now.

MaskRay · 2020-11-30T05:01:23Z

https://reviews.llvm.org/D57319 (LLVM 9) has fixed sw a0, (0)(t3)

https://reviews.llvm.org/D92293 will fix la sp, (_end + 4096) - ((64 / 8) * 36)

allenjbaum · 2021-04-21T02:01:33Z

Is this something that can be closed now?

allenjbaum · 2021-04-26T23:42:23Z

No comments, so I'm closing this. Reopen if someone can show it is still failing.

JunningWu · 2022-02-26T07:50:05Z

Sorry to reopen this issue.
Now I am trying to compile la ra,_f2802x_usdelay-5, which generates a wrong result.
While when I compile la ra,_f2802x_usdelay+0-5, the result is OK.
LLVM version:13.0.1.
Source code:

_f2802x_usdelay:
	addi  sp,sp,-20
	sw    a1,16(sp) 
	csrr  a1,0x7c0
	sw    a1,12(sp)
	csrr  a1,mstatus
	la ra,_f2802x_usdelay+0-5

Disassembly for la ra,_f2802x_usdelay+0-5:

00010000 <_f2802x_usdelay>:
   10000:	1131                	addi	sp,sp,-20
   10002:	c82e                	sw	a1,16(sp)
   10004:	7c0025f3          	csrr	a1,loop
   10008:	c62e                	sw	a1,12(sp)
   1000a:	300025f3          	csrr	a1,mstatus
   1000e:	00000097          	auipc	ra,0x0
   10012:	fed08093          	addi	ra,ra,-19 # fffb <SysPwrCtrlRegs+0x239b>

Disassembly for la ra,_f2802x_usdelay-5:

00010000 <_f2802x_usdelay>:
   10000:	1131                	addi	sp,sp,-20
   10002:	c82e                	sw	a1,16(sp)
   10004:	7c0025f3          	csrr	a1,loop
   10008:	c62e                	sw	a1,12(sp)
   1000a:	300025f3          	csrr	a1,mstatus
   1000e:	00000097          	auipc	ra,0x0
   10012:	ff708093          	addi	ra,ra,-9 # 10005 <_f2802x_usdelay+0x5>

@allenjbaum
@MaskRay

aswaterman · 2022-02-26T08:21:03Z

Not sure whether this is an asm spec bug or an LLVM bug, but it doesn’t seem like a bug in this repo. I suggest raising this issue elsewhere and using your workaround locally (or use GNU tools as a workaround).

allenjbaum · 2022-02-26T08:32:17Z

beat me to it. That is pretty strange LLVM behavior.

…

On Sat, Feb 26, 2022 at 12:21 AM Andrew Waterman ***@***.***> wrote: Sounds like you should open the issue on LLVM, not here. — Reply to this email directly, view it on GitHub <#105 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHPXVJWU256JCMBXPAIIHQLU5CEPVANCNFSM4MN6OXTA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you modified the open/close state.Message ID: ***@***.***>

JunningWu · 2022-02-26T08:33:15Z

OK, I have posted this issue to llvm maillist.

This patch improves compatibility with GNU assembler by adding support for constant immediate in la and lla pseudo instruction, and expanding it in the same way as we currently expands li pseudo instruction. Links to discussion related to the above issue in the community - riscv-non-isa/riscv-arch-test#105 riscv-non-isa/riscv-arch-test#108 riscv-non-isa/riscv-arch-test#106 Reviewed By: craig.topper Differential Revision: https://reviews.llvm.org/D150133

jeremybennett mentioned this issue Apr 22, 2020

Use of pseudo instructions in compliance tests. #106

Closed

jeremybennett mentioned this issue Apr 22, 2020

Clang/LLVM does not support all CSRs used in the compliance test suite #107

Closed

bluewww mentioned this issue Apr 22, 2020

RI5CY's compliance_io.h fails to compile with clang #108

Closed

allenjbaum closed this as completed Apr 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Non-standard assembler usage #105

Non-standard assembler usage #105

jeremybennett commented Apr 22, 2020

aswaterman commented Apr 22, 2020

neelgala commented Apr 22, 2020

jeremybennett commented Apr 22, 2020

neelgala commented Apr 22, 2020

bluewww commented Apr 22, 2020 •

edited

Loading

allenjbaum commented Apr 22, 2020 via email

allenjbaum commented Apr 29, 2020

sequencer commented Nov 30, 2020

MaskRay commented Nov 30, 2020 •

edited

Loading

MaskRay commented Nov 30, 2020

allenjbaum commented Apr 21, 2021

allenjbaum commented Apr 26, 2021

JunningWu commented Feb 26, 2022 •

edited

Loading

aswaterman commented Feb 26, 2022 •

edited

Loading

allenjbaum commented Feb 26, 2022 via email

JunningWu commented Feb 26, 2022

Non-standard assembler usage #105

Non-standard assembler usage #105

Comments

jeremybennett commented Apr 22, 2020

aswaterman commented Apr 22, 2020

neelgala commented Apr 22, 2020

jeremybennett commented Apr 22, 2020

neelgala commented Apr 22, 2020

bluewww commented Apr 22, 2020 • edited Loading

allenjbaum commented Apr 22, 2020 via email

allenjbaum commented Apr 29, 2020

sequencer commented Nov 30, 2020

MaskRay commented Nov 30, 2020 • edited Loading

MaskRay commented Nov 30, 2020

allenjbaum commented Apr 21, 2021

allenjbaum commented Apr 26, 2021

JunningWu commented Feb 26, 2022 • edited Loading

aswaterman commented Feb 26, 2022 • edited Loading

allenjbaum commented Feb 26, 2022 via email

JunningWu commented Feb 26, 2022

bluewww commented Apr 22, 2020 •

edited

Loading

MaskRay commented Nov 30, 2020 •

edited

Loading

JunningWu commented Feb 26, 2022 •

edited

Loading

aswaterman commented Feb 26, 2022 •

edited

Loading