[PATCH] 128-bit ABI for x86_64-w64-mingw32 incorrectly generated

Reid Kleckner rnk at google.com
Thu May 1 11:05:25 PDT 2014


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:1504
@@ +1503,3 @@
+  if (Subtarget->isTargetWin64()) {
+      setOperationAction(ISD::SDIV, MVT::i128, Custom);
+      setOperationAction(ISD::UDIV, MVT::i128, Custom);
----------------
Please use two space indentation here.

================
Comment at: lib/Target/X86/X86ISelLowering.cpp:12528-12534
@@ +12527,9 @@
+SDValue X86TargetLowering::LowerWin64_i128OP(SDValue Op, SelectionDAG &DAG) const {
+  if (!Subtarget->isTargetWin64()) {
+      return SDValue();
+  }
+  EVT VT = Op.getValueType();
+  if (!(VT.isFloatingPoint() || VT.isInteger()) || VT.getSizeInBits() != 128) {
+      return SDValue();
+  }
+
----------------
Anton Korobeynikov wrote:
> No braces here. Same below.
I'd rather hoist the check for win64 + [if]128 out of the function that lowers them and turn these into assertions.

It might be better to strengthen these to assertions, if the result of failing to replace something is a miscompile.

================
Comment at: lib/Target/X86/X86ISelLowering.cpp:12571
@@ +12570,3 @@
+  CallLoweringInfo CLI(InChain,
+                    static_cast<EVT>(MVT::v2i64).getTypeForEVT(*DAG.getContext()),
+                    isSigned, !isSigned, false, true,
----------------
This is copy-pasted from other examples, but I'd whack it with clang-format if you have it easily available.  Otherwise, it's fine.

================
Comment at: lib/Target/X86/X86ISelLowering.cpp:13616
@@ +13615,3 @@
+    if (V.getNode())
+        Results.push_back(V);
+    return;
----------------
Indentation.  Alternatively, move the check for win64 + [fi]128 over the lowering call and drop this check.

http://reviews.llvm.org/D1998






More information about the llvm-commits mailing list