This is an archive of the discontinued LLVM Phabricator instance.

[DAGCombiner] Call ExtendUsesToFormExtLoad in (zext (and (load)))->(and (zextload)) even when the and does not have multiple uses
ClosedPublic

Authored by craig.topper on Feb 7 2018, 11:07 PM.

Details

Summary

Same for the sign extend case.

Currently we check for multiple uses on the binop. Then we call ExtendUsesToFormExtLoad to capture SetCCs that use the load. So we only end up finding any setccs when the and has additional uses and the load is used by a setcc. I don't think the and having multiple uses is relevant here. I think we should only be checking for the load having multiple uses.

This changes an NVPTX test because we now find that the load has a second use by a truncate, but ExtendUsesToFormExtLoad only looks at setccs it can extend. All other operations just check isTruncateFree. Maybe we should allow widening of an existing truncate even if its not free?

I'm also wondering if we need to check isTruncateFree for the truncate we introduce when the and/or/xor has additional users?

Diff Detail

Repository
rL LLVM

Event Timeline

craig.topper created this revision.Feb 7 2018, 11:07 PM

Could we just move the OneUse test inside ExtendUsesToFormExtLoad?

@tra Any comments? Also, a lot of the NVPTX tests can be tricky to maintain as most only use CHECK sparingly, often resulting in tests needing to add additional CHECKs, giving the impression that we've added instructions when they were there 'hidden' all along - would you be open to adding NVPTX support to update_llc_test_checks?

craig.topper retitled this revision from [DAGCombiner] Call ExtendUsesToFormExtLoad in (zext (and (load)))->(and (zextload)) when the load has multiple uses not when the and has multiple uses. to [DAGCombiner] Call ExtendUsesToFormExtLoad in (zext (and (load)))->(and (zextload)) even when the and does not have multiple uses.

Removed the hasOneUse check entirely. The function is just a for loop over the uses that knows to ignore the use that's passed in. If there's only one use, the loop will be one iteration and we'll hit the continue and be done.

Grr its more broken than I realized. We need to pass N0 as the first argument not N. N isn't a user of N0.getOperand(0).

Pass N0 instead of N. But then pass the VT separately so the isTruncateFree call has the right VT to use for its look up since we're no longer passing the extend.

tra accepted this revision.Feb 15 2018, 11:29 AM

nvptx generates identical SASS for both old and new instruction sequence, so this should be benign.

This revision is now accepted and ready to land.Feb 15 2018, 11:29 AM
This revision was automatically updated to reflect the committed changes.