[PATCH] D32916: [DAGCombine] (addcarry 0, 0, X) -> (ext/trunc X)

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

See inline for a few nits, but I think this makes sense now. If I'm seeing the diffs correctly, there was no codegen difference for x86 after adding the 'and' mask, so we must be recognizing and optimizing that pattern. @RKSimon - do you see any other problems?



================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:2148-2149
+  if (isNullConstant(N0) && isNullConstant(N1)) {
+    auto VT = N0.getValueType();
+    auto CarryVT = CarryIn.getValueType();
+    SDValue CarryExt = DAG.getBoolExtOrTrunc(CarryIn, DL, VT, CarryVT);
----------------
It's borderline, but I think we prefer to use the actual type (EVT) in code like this.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:2151
+    SDValue CarryExt = DAG.getBoolExtOrTrunc(CarryIn, DL, VT, CarryVT);
+    AddToWorklist(CarryExt.getNode());
+    return CombineTo(N, DAG.getNode(ISD::AND, DL, VT, CarryExt,
----------------
Do you need to explicitly add to worklist? I thought this gets handled automatically by the combiner.


================
Comment at: test/CodeGen/X86/mul-i1024.ll:5
 
 define void @test_1024(i1024* %a, i1024* %b, i1024* %out) nounwind {
 ; X32-LABEL: test_1024:
----------------
What does testing with i1024 tell us that testing with i512 (or i256 for that matter) would not?
This is a massive file and a massive diff, and I have no idea how to make sense of it. If others can follow what's happening in this codegen, I suppose we should leave it. Otherwise, let's just get rid of this file?


https://reviews.llvm.org/D32916