[PATCH] D24076: [ARM] Use __rt_div functions for DIVREM on Windows

Renato Golin via llvm-commits llvm-commits at lists.llvm.org
Wed Aug 31 07:35:32 PDT 2016


rengolin requested changes to this revision.
rengolin added a comment.
This revision now requires changes to proceed.

Hi Martin,

The fact that divmod-eabi.ll changes is an indication that the interaction between the Dag nodes and the values they produce is weird. I suggest you walk through the back-end and print the Dag before and after the transformations.

One thing that, unfortunately, happens is that Div/Mod patters are passed multiple times over the same nodes, as they get converted from Div/Mod to Divmod, etc. This is particular to ARM and EABI, and may be getting in the way of your lowering. I can't imagine why, since it also returns two values, so it may be some corner case no one knew about.

divmod-eabi.ll is a good first approach to the problem, but you will probably have more corner cases, so please add them to the same file as you add a windows test to it.

cheers,
--renato


================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:800
@@ -800,1 +799,3 @@
+      Subtarget->isTargetGNUAEABI() || Subtarget->isTargetMuslAEABI() ||
+      (Subtarget->isTargetWindows() && !Subtarget->hasDivide())) {
     setOperationAction(ISD::SREM, MVT::i64, Custom);
----------------
hasDivide makes sense, but is unrelated and may break some expectations I don't know about. I'd leave it for later, since this shouldn't hit here if the sub-arch has divide anyway.

================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:805
@@ -803,9 +804,3 @@
 
-    setLibcallName(RTLIB::SDIVREM_I8,  "__aeabi_idivmod");
-    setLibcallName(RTLIB::SDIVREM_I16, "__aeabi_idivmod");
-    setLibcallName(RTLIB::SDIVREM_I32, "__aeabi_idivmod");
-    setLibcallName(RTLIB::SDIVREM_I64, "__aeabi_ldivmod");
-    setLibcallName(RTLIB::UDIVREM_I8,  "__aeabi_uidivmod");
-    setLibcallName(RTLIB::UDIVREM_I16, "__aeabi_uidivmod");
-    setLibcallName(RTLIB::UDIVREM_I32, "__aeabi_uidivmod");
-    setLibcallName(RTLIB::UDIVREM_I64, "__aeabi_uldivmod");
+    if (Subtarget->isTargetWindows()) {
+      setLibcallName(RTLIB::SDIVREM_I8,  "__rt_sdiv");
----------------
I'd have put this in a separate if block, since the function names are *all* different.

================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:12090
@@ -12077,2 +12089,3 @@
   for (unsigned i = 0, e = N->getNumOperands(); i != e; ++i) {
-    EVT ArgVT = N->getOperand(i).getValueType();
+    unsigned src = Subtarget->isTargetWindows() ? e - 1 - i : i;
+    EVT ArgVT = N->getOperand(src).getValueType();
----------------
This is not about windows or not, it's about rt_div instead of eabi_divmod. Please, make it dependent on the RT type (not OS).

Also, this induction fiddling is confusing. The easiest thing to do is to swap the two first arguments before the loop. It's clear and precise.


https://reviews.llvm.org/D24076





More information about the llvm-commits mailing list