This is an archive of the discontinued LLVM Phabricator instance.

[ARM] Update ReconstructShuffle to handle mismatched types
ClosedPublic

Authored by sbaranga on Aug 3 2015, 7:52 AM.

Details

Summary

Port the ReconstructShuffle function from AArch64 to ARM
to handle mismatched incoming types in the BUILD_VECTOR
node.

This fixes an outstanding FIXME in the ReconstructShuffle
code.

Diff Detail

Event Timeline

sbaranga updated this revision to Diff 31232.Aug 3 2015, 7:52 AM
sbaranga retitled this revision from to [ARM] Update ReconstructShuffle to handle mismatched types.
sbaranga updated this object.
sbaranga added a subscriber: llvm-commits.

Ideally this would be shared code with AArch64, but I'm not sure how we would do that (or if it's even a good idea).

Hi Silviu,

This is a good patch, thanks! I have a few comments inline, but it's mostly good.

About the test, you seem to cover a number of new cases and only one is being tested. I'd like to see similar functions with undefs on either places, and also wrongly sized vectors not being transformed, to make sure we're not breaking anything.

cheers,
--renato

lib/Target/ARM/ARMISelLowering.cpp
5605

extra curly braces

5639–5640

It's obviously better to have hard rules, where they're needed, but what if the vector types are odd or larger than expected. Shouldn't we return SDValue() here, instead of blowing up?

Same on the assert just above.

5696

I assume that getNode will fail miserably if the bitcast is not possible / allowed. But I can't think of anything that could go wrong at this stage regarding casts...

5702

Is DEBUG also on during Release+Asserts? This is how most of the buildbots run nowadays, so it'd be good to add this check when in Release mode, too.

I'm assuming that, if you remove the DEBUG(), under O3, the compiler will remove the whole loop if the asserts fail to materialise.

5707

Love this comment! :)

5725–5735

This seems dangerous. Assuming there are only two, then using Sources.size().

I know there is an assert right at the beginning, but this line of code wouldn't know if that number has changed since, or if the assert is removed at a later date.

I'd put another assert here, too.

Hi Renato,

Thanks for the review!

FWIW I've tested this with lnt, spec2000, and some other benchmarks, and everything seems to work.
Getting code to trigger in this stage of codegen is always tricky...

Cheers,
Silviu

lib/Target/ARM/ARMISelLowering.cpp
5639–5640

Yes, good catch! I believe you're correct. I haven't managed to trigger this behaviour from IR though.

5702

Looks like DEBUG is always a nop in non-debug builds. I'll remove the DEBUG statement.

5725–5735

The code itself should be safe (we're not adding more sources from the point where we're checking that we have at most two), but I agree that an assert here is a good idea.

sbaranga updated this revision to Diff 31358.Aug 5 2015, 9:23 AM

Add more regression tests. We now also test vtrn generation.
Also address outstanding review comments.

rengolin accepted this revision.Aug 5 2015, 9:33 AM
rengolin added a reviewer: rengolin.

Perfect, LGTM. Thanks!

This revision is now accepted and ready to land.Aug 5 2015, 9:33 AM
sbaranga closed this revision.Aug 7 2015, 4:41 AM

Thanks! Committed in r244314.