This is an archive of the discontinued LLVM Phabricator instance.

[X86] Add comment string for broadcast loads from the constant pool.
ClosedPublic

Authored by craig.topper on Jul 1 2017, 12:35 AM.

Details

Summary

When broadcasting from the constant pool its useful to print out the final vector similar to what we do for normal moves from the constant pool.

I changed only a couple tests that were broadcast focused. One of them had been previously hand tweaked after running the script so that it could check the constant pool declaration. But I think this patch makes that unnecessary now since we can check the comment instead.

Diff Detail

Repository
rL LLVM

Event Timeline

craig.topper created this revision.Jul 1 2017, 12:35 AM
spatel added inline comments.Jul 1 2017, 7:03 AM
lib/Target/X86/X86MCInstLower.cpp
1863–1867 ↗(On Diff #104980)

Could set NumElts above and then fall-through to avoid the switch-within-a-switch? Same for NumLanes below here.

1905–1909 ↗(On Diff #104980)

Make a helper function, so we're not duplicating so much code?

test/CodeGen/X86/broadcast-elm-cross-splat-vec.ll
2 ↗(On Diff #104980)

Extra/stale comment line.

RKSimon added inline comments.Jul 1 2017, 7:11 AM
test/CodeGen/X86/broadcast-elm-cross-splat-vec.ll
2039 ↗(On Diff #104980)

Don't we have to be careful using NaNs in comments as different compilers/libs print them differently?

craig.topper added inline comments.Jul 1 2017, 1:54 PM
lib/Target/X86/X86MCInstLower.cpp
1863–1867 ↗(On Diff #104980)

How do I fallthrough with different values for NumElts without using a goto?

Reduce code duplication.

I don't think we have to worry about NaN printing differences because we're going through APFloat's toString in this case which I think is stable.

The place we had NaN variation is when we go through raw_otream::operator<<(double) I think. Though that may have been fixed when an isnan check got added to llvm::write_double back in October. Previously I think we may have fallen into the platform's snprintf implementation.

spatel accepted this revision.Jul 2 2017, 6:03 AM

LGTM.

lib/Target/X86/X86MCInstLower.cpp
1863–1867 ↗(On Diff #104980)

Er, yes that would be difficult. :)
If there was a helper function for the remaining chunk of code, then we could pass NumElts to it as a param for each case and remove the switch-within-a-switch?

This revision is now accepted and ready to land.Jul 2 2017, 6:03 AM
This revision was automatically updated to reflect the committed changes.