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

[Luís Marques via Phabricator via llvm-commits]

luismarques added inline comments.


================
Comment at: clang/include/clang/Driver/Options.td:3149-3152
+def mno_div : Flag<["-"], "mno-div">, Group<m_riscv_Features_Group>,
+  HelpText<"Disable hardware integral division instructions in M extension">;
+def mdiv : Flag<["-"], "mdiv">, Group<m_riscv_Features_Group>,
+  HelpText<"Enable hardware integral division instructions in M extension">;
----------------
Remove the redundant "hardware".

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


================
Comment at: clang/lib/Basic/Targets/RISCV.h:49
   bool HasZvlsseg = false;
+  bool DisableHardwareIntDiv = false;
 
----------------
`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.


================
Comment at: clang/test/Driver/riscv-no-div.c:1-17
+// RUN: %clang -target riscv32-unknown-elf %s -mno-div -S -o - 2>&1 \
+// RUN:   | not FileCheck -check-prefix=CHECK-NOERROR %s
+
+// RUN: %clang -target riscv64-unknown-elf %s -mno-div -S -o - 2>&1 \
+// RUN:   | not FileCheck -check-prefix=CHECK-NOERROR %s
+
+// RUN: %clang -target riscv32-unknown-elf %s -mno-div -S -o - 2>&1 \
----------------
Is `not FileCheck` the only way to check these do not occur?


================
Comment at: clang/test/Driver/riscv-no-div.c:33-35
+  // 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]}}
----------------
Should we be checking the actual instructions in a Clang test?


================
Comment at: llvm/test/CodeGen/RISCV/no-div.ll:29-31
+; This test makes sure even when both M extension no-div option enabled,
+; the compile would not fail. Instead, it will use a fallback solution like
+; calling builtin library functions.
----------------
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.


================
Comment at: llvm/test/CodeGen/RISCV/no-div.ll:34-44
+; CHECK-UDIV: divu{{w?}} {{[as]}}{{[0-9]}}, {{[as]}}{{[0-9]}}, {{[as]}}{{[0-9]}}
+  %1 = udiv i32 %a, %b
+; CHECK-DIV: div{{w?}} {{[as]}}{{[0-9]}}, {{[as]}}{{[0-9]}}, {{[as]}}{{[0-9]}}
+  %2 = sdiv i32 %a, %1
+; CHECK-MUL: mul{{w?}} {{[as]}}{{[0-9]}}, {{[as]}}{{[0-9]}}, {{[as]}}{{[0-9]}}
+  %3 = mul i32 %b, %2
+; CHECK-UREM: remu{{w?}} {{[as]}}{{[0-9]}}, {{[as]}}{{[0-9]}}, {{[as]}}{{[0-9]}}
----------------
@jrtc27 should we use update_llc_test_checks.py instead here?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102839/new/

https://reviews.llvm.org/D102839