Page MenuHomePhabricator

[SelectionDAG] Check legality for ADDCARRY in expandMUL_LOHI
AbandonedPublic

Authored by leonardchan on Mar 7 2019, 5:02 PM.

Details

Summary

Add a check for this operation since for X86, it is only supported for 64 bit targets according to the FIXME in X86ISelLowering.cpp:

// Only custom-lower 64-bit SADDO and friends on 64-bit because we don't
// handle type legalization for these operations here.
//
// FIXME: We really should do custom legalization for addition and
// subtraction on x86-32 once PR3203 is fixed.  We really can't do much better
// than generic legalization for 64-bit multiplication-with-overflow, though.

Diff Detail

Event Timeline

leonardchan created this revision.Mar 7 2019, 5:02 PM
RKSimon added inline comments.Mar 11 2019, 8:47 AM
llvm/test/CodeGen/X86/smul_fix.ll
3

Aren't we losing test coverage by dropping this?

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.

RKSimon resigned from this revision.Apr 28 2019, 1:15 AM

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.

Sorry for the delay. Was addressing something else for a bit.

The intention of this was to address an error when compiling for sparc where we get this error during ISEL selection:

LLVM ERROR: Cannot select: t42: i32,i32 = addcarry t41:1, Constant:i32<0>, t90:1

Reproducable from

$ bin/llc < ../llvm-project-3/llvm/test/CodeGen/X86/smul_fix.ll -mtriple=sparc

I don't remember why I talk about X86 since this should have nothing to do with this, but for sparc at least I imagine we shouldn't have this error be raised.

ADDCARRY doesn't seem to get processed through ExpandIntRes_ADDSUBCARRY() (according to the debug dump), but it does try to get handled by ExpandNode() which doesn't have a case for ADDCARRY and fails to expand it. Is the correct way to approach this just to add an ADDCARRY case to ExpandNode()?

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.

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.

Made D61411 to address this.

leonardchan abandoned this revision.May 1 2019, 8:52 PM