[RISCV] -mno-relax: emit .option norelax

[MaskRay]

Summary

GNU as defaults to -mrelax. Emit .option norelax so that the emitted
assembly file can be assembled back correctly in -mno-relax mode.
This matches GCC.

[MaskRay]

improve test

[jrtc27]

I view this as a bug in whatever calls the assembler; it should be passing -mno-relax, just as it needs to pass -fPIC as appropriate.

[jrtc27]

I assume GCC's behaviour dates from before GNU as supported -mno-relax? Which isn't interesting to us, we require a pretty new binutils so %got_pcrel_hi works.

[MaskRay]

In D102535#2760807, @jrtc27 wrote:

I view this as a bug in whatever calls the assembler; it should be passing -mno-relax, just as it needs to pass -fPIC as appropriate.

I'll use the -g analogy. clang -S -g emits debug info and leaves a .debug_line marker so that the assembler knows it needs to synthesize debug info and should not synthesize .debug_info from ELF symbols.
The assembler doesn't need -g to function.

For .option nopic, I agree it is not useful.

[jrtc27]

In D102535#2760899, @MaskRay wrote:

In D102535#2760807, @jrtc27 wrote:

I view this as a bug in whatever calls the assembler; it should be passing -mno-relax, just as it needs to pass -fPIC as appropriate.

I'll use the -g analogy. clang -S -g emits debug info and leaves a .debug_line marker so that the assembler knows it needs to synthesize debug info and should not synthesize .debug_info from ELF symbols.
The assembler doesn't need -g to function.

For .option nopic, I agree it is not useful.

The two aren't comparable. Relaxation affects assembling and not code generation. Debug info affects code generation to emit all the information that's otherwise lost (the assembly doesn't have a copy of your source code...). Even PIC is more useful than .option norelax, as it affects what instructions get used to lower global addresses etc.

[MaskRay]

In D102535#2760911, @jrtc27 wrote:

In D102535#2760899, @MaskRay wrote:

In D102535#2760807, @jrtc27 wrote:

I view this as a bug in whatever calls the assembler; it should be passing -mno-relax, just as it needs to pass -fPIC as appropriate.

I'll use the -g analogy. clang -S -g emits debug info and leaves a .debug_line marker so that the assembler knows it needs to synthesize debug info and should not synthesize .debug_info from ELF symbols.
The assembler doesn't need -g to function.

For .option nopic, I agree it is not useful.

The two aren't comparable. Relaxation affects assembling and not code generation. Debug info affects code generation to emit all the information that's otherwise lost (the assembly doesn't have a copy of your source code...). Even PIC is more useful than .option norelax, as it affects what instructions get used to lower global addresses etc.

I think the -Wa,-mno-relax argument is justified if code generation cannot affect relaxation (which I am unsure).

Can GCC base assembly emission decisions on -mno-relax?

[jrtc27]

In D102535#2760944, @MaskRay wrote:

In D102535#2760911, @jrtc27 wrote:

In D102535#2760899, @MaskRay wrote:

In D102535#2760807, @jrtc27 wrote:

I view this as a bug in whatever calls the assembler; it should be passing -mno-relax, just as it needs to pass -fPIC as appropriate.

I'll use the -g analogy. clang -S -g emits debug info and leaves a .debug_line marker so that the assembler knows it needs to synthesize debug info and should not synthesize .debug_info from ELF symbols.
The assembler doesn't need -g to function.

For .option nopic, I agree it is not useful.

The two aren't comparable. Relaxation affects assembling and not code generation. Debug info affects code generation to emit all the information that's otherwise lost (the assembly doesn't have a copy of your source code...). Even PIC is more useful than .option norelax, as it affects what instructions get used to lower global addresses etc.

I think the -Wa,-mno-relax argument is justified if code generation cannot affect relaxation (which I am unsure).

Can GCC base assembly emission decisions on -mno-relax?

It can if it wants to, but as far as I know the option exists solely for the driver to pass to the assembler, and I don't know what you'd do with the information that's meaningful.

[raj.khem]

In D102535#2760944, @MaskRay wrote:

In D102535#2760911, @jrtc27 wrote:

In D102535#2760899, @MaskRay wrote:

In D102535#2760807, @jrtc27 wrote:

I view this as a bug in whatever calls the assembler; it should be passing -mno-relax, just as it needs to pass -fPIC as appropriate.

I'll use the -g analogy. clang -S -g emits debug info and leaves a .debug_line marker so that the assembler knows it needs to synthesize debug info and should not synthesize .debug_info from ELF symbols.
The assembler doesn't need -g to function.

For .option nopic, I agree it is not useful.

The two aren't comparable. Relaxation affects assembling and not code generation. Debug info affects code generation to emit all the information that's otherwise lost (the assembly doesn't have a copy of your source code...). Even PIC is more useful than .option norelax, as it affects what instructions get used to lower global addresses etc.

I think the -Wa,-mno-relax argument is justified if code generation cannot affect relaxation (which I am unsure).

Can GCC base assembly emission decisions on -mno-relax?

I think it will be better that clang driver passed the right argument to assembler ( provided it is external GNU assembler )

[MaskRay]

This is to make clang -S -mno-relax a.c; clang -S a.s use no-relax.

I feel that if -mno-relax is really an assembler only option, -Wa,-mno-relax would be more appropriate. And -mno-relax should be warned with -S.

However, if the compiler (either Clang or GCC) reserves rights to change code generation (likely (as I feel), as it may do something special to aid assemblers), I'll actually prefer .option norelax in the emitted assembly output.
(With this patch, we don't need to pass -mno-relax to GNU as for the -fno-integrated-as code path.)

I found a random issue discussing relaxation (https://github.com/riscv/riscv-elf-psabi-doc/issues/34) and asked the code generation question to GCC contributors there.

[compnerd]

If the user is passing along -mno-relax encoding that in the object file isn't expensive, and it doesn't harm anything. In fact, the user _could_ at a later point pass in -mrelax to the assembler to override the beahviour (with a warning). But what @raj.khem states about the non-IAS case should hold - -mno-relax should get passed along to the assembler along with the hint in the object file.

[jrtc27]

In D102535#2769528, @compnerd wrote:

If the user is passing along -mno-relax encoding that in the object file isn't expensive, and it doesn't harm anything. In fact, the user _could_ at a later point pass in -mrelax to the assembler to override the beahviour (with a warning). But what @raj.khem states about the non-IAS case should hold - -mno-relax should get passed along to the assembler along with the hint in the object file.

.option norelax overrides -mrelax, just like any other assembler directive.

[compnerd]

Hmm, actually looking at gcc, it appears that this is actually matching semantics with gcc:
https://github.com/riscv/riscv-gcc/blob/03cb20e5433cd8e65af6a1a6baaf3fe4c72785f6/gcc/config/riscv/riscv.c#L4590-L4593

In D102535#2769537, @jrtc27 wrote:

In D102535#2769528, @compnerd wrote:

If the user is passing along -mno-relax encoding that in the object file isn't expensive, and it doesn't harm anything. In fact, the user _could_ at a later point pass in -mrelax to the assembler to override the beahviour (with a warning). But what @raj.khem states about the non-IAS case should hold - -mno-relax should get passed along to the assembler along with the hint in the object file.

.option norelax overrides -mrelax, just like any other assembler directive.

Ah right, the attributes are processed late in ELF.

[MaskRay]

@asb @craig.topper @luismarques opinions on .option norelax and https://github.com/riscv/riscv-elf-psabi-doc/issues/34#issuecomment-844548202 ?

(From that comment it seems that https://github.com/riscv/riscv-c-api-doc is the place to discuss the option but that repo doesn't have many watchers.)

[kito-cheng]

Personally I prefer keep compatible with GNU toolchain behavior on this, so this patch is LGTM.

But I would like to let @asb and @luismarques to do the final judge.

[kito-cheng]

After discussion with Jim W, we think it's OK to pass -mno-relax to as for GCC, and we could sent patch recently, but keep emitting .option norelax for this moment to make sure the compatibility, and remove that later after 1 or 2 GCC version (that's means 1~2 years in generally)

[kito-cheng]

GCC pass -mno-relax to assembler now: https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=3b0a7d624e64eeb81e4d5e8c62c46d86ef521857

[MaskRay]

In D102535#2779469, @kito-cheng wrote:

GCC pass -mno-relax to assembler now: https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=3b0a7d624e64eeb81e4d5e8c62c46d86ef521857

I think an assembler only option should be spelled as -Wa,*. The driver option isn't useful and should just be avoided.

For -mno-relax, I think there can be arguments that the assembler generation may be affected (to appease assembler, for example) so a driver option is needed.

If the driver option -mno-relax is still preferred over -Wa,-mno-relax, I can abandon this and submit another one passing -mno-relax to GNU as for -fno-integrated-as.

[kito-cheng]

I think an assembler only option should be spelled as -Wa,*. The driver option isn't useful and should just be avoided.

For -mno-relax, I think there can be arguments that the assembler generation may be affected (to appease assembler, for example) so a driver option is needed.

If the driver option -mno-relax is still preferred over -Wa,-mno-relax, I can abandon this and submit another one passing -mno-relax to GNU as for -fno-integrated-as.

Jim W and me don't have strong opinion here, but we don't want to break backward compatible/behavior change on GNU toolchain, so we using both for now.

The reason why we did't pass -mno-relax to assembler...is because we already using .option norelax scheme there, so we thought it's enough (long times ago I don't even remember when).

For clang/LLVM, I think there is freedom to choose implement both or just pass -mno-relax.

[MaskRay]

If .option norelax is the agreed-upon choice (is it?), can I get a stamp?

I did not participate in the meetings you have ... and I am confused about the state now.

[kito-cheng]

If .option norelax is the agreed-upon choice (is it?), can I get a stamp?

I did not participate in the meetings you have ... and I am confused about the state now.

Oh, sorry for confusing, I should ask that in LLVM sync up meeting before I said it's free to choose, we can discuss this at today's LLVM sync up meeting,
But again, I would prefer to let @asb and @luismarques to do the final judge :)

[luismarques]

In D102535#2783915, @kito-cheng wrote:

Oh, sorry for confusing, I should ask that in LLVM sync up meeting before I said it's free to choose, we can discuss this at today's LLVM sync up meeting,
But again, I would prefer to let @asb and @luismarques to do the final judge :)

@evandro had some good points about this issue that we didn't have time to fully discuss during the call. Evandro, when you find the time please chime in with your points here. Thank you!

[luismarques]

In D102535#2784970, @luismarques wrote:

@evandro had some good points about this issue that we didn't have time to fully discuss during the call. Evandro, when you find the time please chime in with your points here. Thank you!

ping @evandro. IIRC correctly the main argument was that this was an optional optimization option, so it should be chosen explicitly (or not) when driving the assembler.

[evandro]

Methinks that the choice of whether to relax or not should be decided outside of the assembler code. This directive sets the choice in stone and counters any choice done at build time, when optimizations are specified. For instance, when building a debug version, the optimizations may be different from when building a release version. This directive blocks such common practices and may create confusion and non expected results.

[luismarques]

In D102535#2811903, @evandro wrote:

Methinks that the choice of whether to relax or not should be decided outside of the assembler code. This directive sets the choice in stone and counters any choice done at build time, when optimizations are specified. For instance, when building a debug version, the optimizations may be different from when building a release version. This directive blocks such common practices and may create confusion and non expected results.

I thought I understood your point initially, but now I'm not sure. Can you clarify what flexibility you're losing? Doesn't the emission of this directive reflect exactly the build settings you specify at compilation time? Why is it important to override this later?

[evandro]

In D102535#2819032, @luismarques wrote:

I thought I understood your point initially, but now I'm not sure. Can you clarify what flexibility you're losing? Doesn't the emission of this directive reflect exactly the build settings you specify at compilation time? Why is it important to override this later?

If consistency is desired from clang to as, then -Wa,-mno-relax is preferred. This way, the same assembly source can be assembled with or without relaxation without changing the source code. Moreover, in the case of an assembly source file, the choice to relax its code or not can be made at build time, or, more specifically, at link time.

llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp

bool RISCVAsmPrinter::runOnMachineFunction(MachineFunction &MF) {
   STI = &NewSTI;
 
   SetupMachineFunction(MF);
   emitFunctionBody();
   return false;
 }
 
 void RISCVAsmPrinter::emitStartOfAsmFile(Module &M) {
+  if (!STI->getFeatureBits()[RISCV::FeatureRelax]) {
+    static_cast<RISCVTargetStreamer &>(*OutStreamer->getTargetStreamer())
+        .emitDirectiveOptionNoRelax();
+  }
+
   if (TM.getTargetTriple().isOSBinFormatELF())
     emitAttributes();
 }
 
 void RISCVAsmPrinter::emitEndOfAsmFile(Module &M) {
   RISCVTargetStreamer &RTS =
       static_cast<RISCVTargetStreamer &>(*OutStreamer->getTargetStreamer());
 

llvm/test/CodeGen/RISCV/option-norelax2.ll

+; RUN: llc -mtriple=riscv32 -mattr=-relax < %s | FileCheck %s
+; RUN: llc -mtriple=riscv64 -mattr=-relax < %s | FileCheck %s
+
+; RUN: llc -mtriple=riscv64 -mattr=+relax < %s | FileCheck %s --check-prefix=RELAX
+
+; CHECK: .option norelax
+
+; RELAX-NOT: .option norelax
+
+define void @foo() {
+  ret void
+}