In D55720#1506687, @bjope wrote:

LGTM! (if all comments from other reviewers has been taken care of) (maybe you should wait another day to see if anyone else object, but I think this patch has been open for a long time so there have been plenty of time for comments already)

Btw, I'm still having a hard time reviewing the X86 test cases with very long results, and a little bit worried that those just will give annoying churn when doing unrelated patches in the future, rather than help out detecting problems related to smul.fix. I currently have no more ideas on how to improve that.

Hopefully I'll be able to run some runtime comparison tests between X86 and our OOT target when this has landed (and when I've adapted our target to use these new intrinsics using "legal" lowering and not "expand").

In D51329#1499307, @mstorsjo wrote:

This caused a regression when including mingw-w64 headers (in a case where they happen to include redundant/duplicate attribute specifications), triggering asserts. See https://bugs.llvm.org/show_bug.cgi?id=41852 for details.

Hi, it seems that this patch results in an assertion error when building zircon:

In D51329#1498193, @miyuki wrote:

This caused https://bugs.llvm.org/show_bug.cgi?id=41835
Could you please look into it?

Sorry for the holdup.

In D51329#1488918, @JDevlieghere wrote:

I think this is change is causing an assertion to be hit:

http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/25103/consoleFull#-239233992d489585b-5106-414a-ac11-3ff90657619c
http://green.lab.llvm.org/green/job/clang-stage1-configure-RA/56122/console

Can you have a look?

In D51329#1488465, @rsmith wrote:

Thanks, looks great!

I think the handling of the expansion location in the MacroQualifiedTypeLoc isn't quite right yet (it looks like it will never actually be set at all, because the code to set it is not reachable) but it's also unused at the moment, so I'm happy with that being fixed as a follow-up change after you land this.

*ping*

In D61411#1487156, @efriedma wrote:

LGTM

Please run some basic sanity tests before you merge to make sure this works correctly at runtime. The code looks right, but I don't really trust that. :) If you want to test something simpler than smul.fix, might make sense to just temporarily hack legalization for addition to use this codepath, or something like that, so you can easily understand what you're actually testing.

In D59119#1486753, @efriedma wrote:

If this is fixing a SPARC issue, please add an appropriate testcase for the SPARC backend.

If iN ADDCARRY isn't legal, but iN is legal, it makes sense to handle that in SelectionDAGLegalize::ExpandNode, yes. I thought we had that code already, but I guess not. We should eventually implement ADDCARRY support for SPARC, but the code in SelectionDAGLegalize::ExpandNode will be useful for other targets which don't have an ADDCARRY-like instruction, like RISCV.

In D59119#1424657, @efriedma wrote:

I'm not sure what you're hoping to accomplish with this change. Even on targets where i64 ADDCARRY isn't legal, we have code to expand it (see DAGTypeLegalizer::ExpandIntRes_ADDSUBCARRY etc.), and it's still generally cheap.

*ping*

Ok. Formally requesting for code reviews now.

Uploading my latest version of this patch and reporting some results. Will proceed with formatting and cleanup and ask for official reviews later.