This is an archive of the discontinued LLVM Phabricator instance.

[ARM] SMULW [T|B] DAG Combine
ClosedPublic

Authored by samparker on Mar 7 2017, 11:11 AM.

Details

Summary

Splitting SMULWB and SMULWT instruction selection changes from D30044 into separate patch. ISel has been moved from DAGToDAG into combine and so the related SMLAWB and SMLAWT instructions can be selected in tablegen. The helper functions have been removed from the original patch too.

Diff Detail

Event Timeline

samparker created this revision.Mar 7 2017, 11:11 AM

Please add some testcases covering the improved isS16 function.

lib/Target/ARM/ARMISelLowering.cpp
1482

Add a comment describing what exactly this function is checking for? It's not obvious why a function called "isS16" is special-casing SRA nodes.

9967

getResNo, maybe?

11835

You could also simplify SMULWT nodes in a similar manner.

samparker updated this revision to Diff 91004.Mar 8 2017, 6:15 AM

Hi Eli,

I've added a comment to isS16, slightly simplified the node selection and added a simplifyTop16 function for SMULWT. I've also added a couple of test for loading i16 values.

cheers,
sam

efriedma added inline comments.Mar 8 2017, 12:08 PM
lib/Target/ARM/ARMISelLowering.cpp
1497

Explicitly passing "true" is probably a bad idea here.

1499

You probably want to use TargetLowering::SimplifyDemandedBits rather than TargetLoweringOpt::SimplifyDemandedBits; the latter has weird handling for nodes with multiple uses.

I count eight nearly identical copies of this code, with just varying demanded bits; two here, combineBT in X86ISelLowering, two in XCoreTargetLowering::PerformDAGCombine, SITargetLowering::performCvtF32UByteNCombine, performTBISimplification in AArch64ISelLowering.cpp, simplifyI24 in AMDGPUISelLowering.cpp. Please add a helper to TargetLowering for all of these to use. (Don't worry about fixing the other copies in this patch.)

samparker updated this revision to Diff 91146.Mar 9 2017, 2:06 AM

Moved the simplify helper functions into TargetLowering.

efriedma added inline comments.Mar 9 2017, 9:51 AM
lib/CodeGen/SelectionDAG/TargetLowering.cpp
475 ↗(On Diff #91146)

You can unify SimplifyDemandedLowBits and SimplifyDemandedHighBits by adding one function that takes the DemandedMask as an argument.

Yes ok, I had hoped that more code could be refactored out of the other backends since most of the uses were for low bit masks, but it makes more sense to a flexible API. This makes the interface very similar to TLO::SimplifyDemandedBits, do you know why that function is set to assume a single use?

samparker updated this revision to Diff 91291.Mar 10 2017, 1:47 AM

Merged the two helper function into one, using an APInt for the mask.

efriedma accepted this revision.Mar 13 2017, 5:49 PM

LGTM.

This makes the interface very similar to TLO::SimplifyDemandedBits, do you know why that function is set to assume a single use?

The function has exactly one user in lib/Target/AMDGPU/AMDGPUISelLowering.cpp, and the behavior is necessary there. (I haven't really carefully considered the consequences of it.)

This revision is now accepted and ready to land.Mar 13 2017, 5:49 PM
This revision was automatically updated to reflect the committed changes.