[RISCV][Clang] Add -mno-div option to disable hardware int division

[ksyx]

Summary

With option -mno-div, hardware integral division and modulus instructions in M extension will be disabled, while leaving multiply instructions enabled.

I am not sure whether there is a better way of implementing this, so please point out if there is one, thanks.

Reference: riscv-toolchain-conventions

[craig.topper]

Isn't the gcc name for this -mno-div? Should we be consistent?

clang/lib/Basic/Targets/RISCV.cpp

154 ↗

(On Diff #346687)

Does gcc also have this define? Why do we need this in addition to not defining __riscv_div.

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

216 ↗

(On Diff #346687)

These MUL lines are affected by the disableHardwareIntDiv above. Please split mul and div handling apart.

[jrtc27]

I continue to believe this kind of thing is a bad idea (missing fsqrt for F is another one). M means mul and div, if your hardware lacks div then it's not compliant with the spec. M-mode is free to lie to S-mode and above as to what's supported by the hardware and emulate as needed, but if misa.M is 1 then div must work.

[ksyx]

In D102839#2775732, @craig.topper wrote:

Isn't the gcc name for this -mno-div? Should we be consistent?

I am not sure whether using -mno-div might bring in some confusion since from the name no-div itself it expresses a sense of excluding all hardware division instructions, both float ones and integral ones, from my personal understanding. However, as in the documentation, it only affects the integral ones and float one was controlled by another. Thus for the clearance I used this naming.

I have not much understanding about what compatibility with GCC we need to achieve but I am okay with both kinds of naming and changing this is totally ok if required.

clang/lib/Basic/Targets/RISCV.cpp

154 ↗

(On Diff #346687)

Ok. I will remove it.

[ksyx]

• remove redundant macro definition
• split mul and div handling in RISCVISelLowering.cpp
• rename no-idiv into no-div

[ksyx]

fix: indent

[luismarques]

clang/include/clang/Driver/Options.td

3149–3152

Remove the redundant "hardware".

BTW, is "integral" actually correct, or should it be "integer division"?

clang/lib/Basic/Targets/RISCV.h

49 ↗

(On Diff #347637)

DisableHardwareIntDiv -> DisableIntDiv

Also, in general it would be best for the feature to be positive rather than negative, but I'm not sure whether it makes sense to make an exception here, since the option that toggles the feature is also negative.

clang/test/Driver/riscv-no-div.c

2–18

Is not FileCheck the only way to check these do not occur?

34–36

Should we be checking the actual instructions in a Clang test?

llvm/test/CodeGen/RISCV/no-div.ll

29–31 ↗

(On Diff #347637)

I'm not sure it makes sense to refer to the possibility of the compilation failing. It's just that we shouldn't emit divide instructions.

34–44 ↗

(On Diff #347637)

@jrtc27 should we use update_llc_test_checks.py instead here?

[kito-cheng]

We have Zmmul extension in the ISA spec now, that's equivalent to -mno-div , so I suggest we should go forward to implement that extension rather than -mno-div.

https://github.com/riscv/riscv-isa-manual/pull/648

[jrtc27]

In D102839#2782557, @kito-cheng wrote:

We have Zmmul extension in the ISA spec now, that's equivalent to -mno-div , so I suggest we should go forward to implement that extension rather than -mno-div.

https://github.com/riscv/riscv-isa-manual/pull/648

Yes, if people want subsets of extensions, that's the way to do it.

[luismarques]

In D102839#2782557, @kito-cheng wrote:

We have Zmmul extension in the ISA spec now, that's equivalent to -mno-div , so I suggest we should go forward to implement that extension rather than -mno-div.
https://github.com/riscv/riscv-isa-manual/pull/648

Thanks for bringing that to our attention, Kito. I had missed that recent PR and its discussion.
If the GNU ecosystem intends to move away from -mno-div soonish then Zmmul is for sure the way to go. I don't see enough benefit for LLVM to add a stop-gap compatibility option.

[ksyx]

So it seems the better way to do this would definitely by adding a subextension as the spec had changed. But I'd like also to ask how will GCC deal with this option, and should we make this option an alias to turn off M extension and turn on ZMMul extension?

[luismarques]

In D102839#2783636, @ksyx wrote:

So it seems the better way to do this would definitely by adding a subextension as the spec had changed. But I'd like also to ask how will GCC deal with this option, and should we make this option an alias to turn off M extension and turn on ZMMul extension?

Regarding "turn off M extension", see Krste's comment in the PR:

I think -mno-div is OK as a compiler option, but it's meaning is for the compiler not to generate divide instructions, not to indicate what the ISA is. So for example, -mno-div should not set ELF flags in binary to indicate ISA doesn't have divide - it should simply not generate divide instructions.

[ksyx]

Implemented Zmmul subextension, and let no-div to be using this subextension instead of M itself.

[ksyx]

In D102839#2784197, @luismarques wrote:

In D102839#2783636, @ksyx wrote:

So it seems the better way to do this would definitely by adding a subextension as the spec had changed. But I'd like also to ask how will GCC deal with this option, and should we make this option an alias to turn off M extension and turn on ZMMul extension?

Regarding "turn off M extension", see Krste's comment in the PR:

I think -mno-div is OK as a compiler option, but it's meaning is for the compiler not to generate divide instructions, not to indicate what the ISA is. So for example, -mno-div should not set ELF flags in binary to indicate ISA doesn't have divide - it should simply not generate divide instructions.

Thanks for mentioning that! Now, I changed the effect of no-div option into choosing a proper extension and implemented the Zmmul subextension. Is this solution acceptable or are there anything need further changes?

[luismarques]

In D102839#2784275, @ksyx wrote:

Thanks for mentioning that! Now, I changed the effect of no-div option into choosing a proper extension and implemented the Zmmul subextension. Is this solution acceptable or are there anything need further changes?

It seems like the community is quickly converging on just using the ISA string with Zmmul, and not using no-div. While being compatible with the GNU tools is nice, if they are planning on dropping support for no-div soonish then we probably shouldn't add support for it. IMO, Zmmul should be a separate patch and presumably (at the moment) be gated by -menable-experimental-extensions.

[kito-cheng]

Personally I prefer to deprecate -mno-div soon, but based on the rule for RISC-V GNU toolchain, it need to wait Zmmul extension frozen.
My plan is deprecate the -mno-div and emit warning to tell user should use Zmmul instead once it frozen.

[asb]

In D102839#2784585, @kito-cheng wrote:

Personally I prefer to deprecate -mno-div soon, but based on the rule for RISC-V GNU toolchain, it need to wait Zmmul extension frozen.
My plan is deprecate the -mno-div and emit warning to tell user should use Zmmul instead once it frozen.

Thanks for clarifying Kito. In that case, I think having clang support -mno-div in order to match the GNU toolchain makes sense, and we can later follow GCC in deprecating it if they do. Given how long it can take to ratify extensions, I'm wary of making assumptions about when zmmul may be ratified.

[ksyx]

In D102839#2784297, @luismarques wrote:

In D102839#2784275, @ksyx wrote:

Thanks for mentioning that! Now, I changed the effect of no-div option into choosing a proper extension and implemented the Zmmul subextension. Is this solution acceptable or are there anything need further changes?

It seems like the community is quickly converging on just using the ISA string with Zmmul, and not using no-div. While being compatible with the GNU tools is nice, if they are planning on dropping support for no-div soonish then we probably shouldn't add support for it. IMO, Zmmul should be a separate patch and presumably (at the moment) be gated by -menable-experimental-extensions.

Thanks for the news and I think having experimental-zmmul a separate patch is a good idea. But which implementation of no-div is better, the one that chooses extension between M and zmmul or the one that simply being an attribute like Diff 3, or are there any other suggestions?

[ksyx]

split Zmmul implementation and -mno-div implementation

clang/include/clang/Driver/Options.td

def mno_relax : Flag<["-"], "mno-relax">, Group<m_Group>,
   HelpText<"Disable linker relaxation">;
 def msmall_data_limit_EQ : Joined<["-"], "msmall-data-limit=">, Group<m_Group>,
   Alias<G>,
   HelpText<"Put global and static data smaller than the limit into a special section">;
 def msave_restore : Flag<["-"], "msave-restore">, Group<m_riscv_Features_Group>,
   HelpText<"Enable using library calls for save and restore">;
 def mno_save_restore : Flag<["-"], "mno-save-restore">, Group<m_riscv_Features_Group>,
   HelpText<"Disable using library calls for save and restore">;
+def mno_div : Flag<["-"], "mno-div">, Group<m_riscv_Features_Group>,
+  HelpText<"Disable integer division instructions in M extension">;
+def mdiv : Flag<["-"], "mdiv">, Group<m_riscv_Features_Group>,
+  HelpText<"Enable integer division instructions in M extension">;
 def mcmodel_EQ_medlow : Flag<["-"], "mcmodel=medlow">, Group<m_riscv_Features_Group>,
   Flags<[CC1Option]>, Alias<mcmodel_EQ>, AliasArgs<["small"]>,
   HelpText<"Equivalent to -mcmodel=small, compatible with RISC-V gcc.">;
 def mcmodel_EQ_medany : Flag<["-"], "mcmodel=medany">, Group<m_riscv_Features_Group>,
   Flags<[CC1Option]>, Alias<mcmodel_EQ>, AliasArgs<["medium"]>,
   HelpText<"Equivalent to -mcmodel=medium, compatible with RISC-V gcc.">;
 def menable_experimental_extensions : Flag<["-"], "menable-experimental-extensions">, Group<m_Group>,
   HelpText<"Enable use of experimental RISC-V extensions.">;

clang/lib/Driver/ToolChains/Arch/RISCV.cpp

void riscv::getRISCVTargetFeatures(const Driver &D, const llvm::Triple &Triple,
   if (Args.hasFlag(options::OPT_msave_restore, options::OPT_mno_save_restore, false))
     Features.push_back("+save-restore");
   else
     Features.push_back("-save-restore");
 
   // Now add any that the user explicitly requested on the command line,
   // which may override the defaults.
   handleTargetFeaturesGroup(Args, Features, options::OPT_m_riscv_Features_Group);
+
+  bool noDiv = Args.hasFlag(options::OPT_mno_div, options::OPT_mdiv, false);
+  for (auto &Feature : Features) {
+    if (noDiv && (Feature == "+m" || Feature == "-div")) {
+      Feature = "+experimental-zmmul";
+    } else if (Feature == "+div") {
+      Feature = "+m";
+    }
+  }
 }
 
 StringRef riscv::getRISCVABI(const ArgList &Args, const llvm::Triple &Triple) {
   assert((Triple.getArch() == llvm::Triple::riscv32 ||
           Triple.getArch() == llvm::Triple::riscv64) &&
          "Unexpected triple");
 
   // GCC's logic around choosing a default `-mabi=` is complex. If GCC is not

clang/test/Driver/riscv-no-div.c

+// RUN: %clang -target riscv32-unknown-elf %s -mno-div -S -o - 2>&1 \
+// RUN:   | not FileCheck -check-prefix=CHECK-DIV %s
+
+// RUN: %clang -target riscv64-unknown-elf %s -mno-div -S -o - 2>&1 \
+// RUN:   | not FileCheck -check-prefix=CHECK-DIV %s
+
+// RUN: %clang -target riscv32-unknown-elf %s -mno-div -S -o - 2>&1 \
+// RUN:   | not FileCheck -check-prefix=CHECK-REM %s
+
+// RUN: %clang -target riscv64-unknown-elf %s -mno-div -S -o - 2>&1 \
+// RUN:   | not FileCheck -check-prefix=CHECK-REM %s
+
+// RUN: %clang -target riscv32-unknown-elf %s -mno-div -S -o - 2>&1 \
+// RUN:   | FileCheck -check-prefix=CHECK-MUL %s
+
+// RUN: %clang -target riscv64-unknown-elf %s -mno-div -S -o - 2>&1 \
+// RUN:   | FileCheck -check-prefix=CHECK-MUL %s
+
+// RUN: %clang -target riscv32-unknown-elf %s -S -o - 2>&1 \
+// RUN:   | FileCheck -check-prefixes=CHECK-MUL,CHECK-DIV,CHECK-REM %s
+
+// RUN: %clang -target riscv64-unknown-elf %s -S -o - 2>&1 \
+// RUN:   | FileCheck -check-prefixes=CHECK-MUL,CHECK-DIV,CHECK-REM %s
+
+int foo(int x, int y) {
+  // CHECK-DIV: div{{w?}} a{{[0-9]}}, a{{[0-9]}}, a{{[0-9]}}
+  // CHECK-REM: rem{{w?}} a{{[0-9]}}, a{{[0-9]}}, a{{[0-9]}}
+  // CHECK-MUL: mul{{w?}} a{{[0-9]}}, a{{[0-9]}}, a{{[0-9]}}
+  return (x / y) * (x % y);
+}