Page MenuHomePhabricator

[LegalizeDAG] Truncate condition operand of ISD::SELECT
ClosedPublic

Authored by evgeny777 on Jan 31 2018, 7:05 AM.

Details

Summary

I'm facing a crash when compiling large in-house project for AArch64 using clang, which happens because condition argument of ISD::SELECT can't be expanded (for my case required expansion is i128 -> i64). This patch adds required support.

Diff Detail

Repository
rL LLVM

Event Timeline

evgeny777 created this revision.Jan 31 2018, 7:05 AM

It seems a little dubious to create a SELECT where the condition isn't either an i1 or a naturally promoted i1... maybe it would make sense to restrict that instead? (And fix the VSELECT expansion to just truncate the condition instead of trying to get fancy.)

lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp
3118 ↗(On Diff #132172)

Probably want to clarify that you're intentionally throwing away the high bits because the operand is required to conform to getBooleanContents.

evgeny777 updated this revision to Diff 132335.Feb 1 2018, 12:49 AM
evgeny777 added a reviewer: efriedma.

Addressed review comments. The new version also handles scalar types which size is not power of 2.

evgeny777 retitled this revision from [LegalizeDAG] Support expanding condition operand of ISD::SELECT to [LegalizeDAG] Truncate condition operand of ISD::SELECT.Feb 1 2018, 1:40 AM
delena added inline comments.Feb 5 2018, 10:58 PM
lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp
368 ↗(On Diff #132335)

Can the condition be wider than getSetCCResultType?

test/CodeGen/AArch64/expand-select.ll
4 ↗(On Diff #132335)

I think you should put here regular CHECKs, like in all other llc tests.

evgeny777 added inline comments.Feb 6 2018, 4:16 AM
lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp
368 ↗(On Diff #132335)

Yes, if this wider type is legal for target architecture. However see @efriedma comment above.

If boolean type is not legal then you either crash when trying to expand condition or when trying to promote it (if type bit width is not power of 2). Here I chose to truncate it to size of getSetCCResultType to avoid promoting it back in PromoteTargetBoolean.

test/CodeGen/AArch64/expand-select.ll
4 ↗(On Diff #132335)

Any suggestion on what should be checked? Probably debug output from -debug-only=selectiondag,isel?

delena added inline comments.Feb 6 2018, 4:28 AM
lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp
368 ↗(On Diff #132335)

My question was incorrect. I wanted to ask whether the getSetCCResultType may be wider than Cond. Do you need to extend the Cond in this case?

test/CodeGen/AArch64/expand-select.ll
4 ↗(On Diff #132335)

just use utils/update_llc_test_checks.py to generate "CHECKS":

utils/update_llc_test_checks.py --llc-binary=<path>/llc test/CodeGen/AArch64/expand-select.ll

evgeny777 added inline comments.Feb 6 2018, 8:03 AM
lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp
368 ↗(On Diff #132335)

I don't think so. PromoteIntOp_SELECT will do this for me. It calls PromoteTargetBoolean which in turn zero extends condition to the size of getSetCCResultType

test/CodeGen/AArch64/expand-select.ll
4 ↗(On Diff #132335)

Thanks, I'll give it a try.

evgeny777 updated this revision to Diff 133037.Feb 6 2018, 10:10 AM

Added checks to test case

delena accepted this revision.Feb 6 2018, 11:32 AM
This revision is now accepted and ready to land.Feb 6 2018, 11:32 AM
efriedma accepted this revision.Feb 6 2018, 11:51 AM
This revision was automatically updated to reflect the committed changes.