Page MenuHomePhabricator

Check bool attribute value in getOptionalBoolLoopAttribute.
ClosedPublic

Authored by asbirlea on Jan 25 2019, 2:42 PM.

Details

Summary

Check the bool value of the attribute in getOptionalBoolLoopAttribute
not just its existance. If the llvm.loop.vectorize exists, but is set to false, hasVectorizeTransformation would return TM_ForcedByUser instead of TM_SuppressedByUser.
This eliminates the warning noise generated when vectorization is explicitly disabled.

Diff Detail

Repository
rL LLVM

Event Timeline

asbirlea created this revision.Jan 25 2019, 2:42 PM

The check hasVectorizeTransformation(L) in front checks whether vectorization is forced by the user. It is not necessary that the user also fixes the simd width/interleave count. Hence, the warning is valid in this case. It is the same behavior as before rL348944 (which I tried to replicate).

test/Transforms/LoopVectorize/X86/vectorization-remarks-missed.ll
36 ↗(On Diff #183626)

Assuming the comment above accurately describes the IR, why do you think the warning should not be emitted?

asbirlea updated this revision to Diff 183960.Jan 28 2019, 2:32 PM

Mistaken source of the noise, the bug I was chasing was in getOptionalBoolLoopAttribute, called by hasVectorizeTransformation(L).
If the llvm.loop.vectorize exists, but is set to false, hasVectorizeTransformation would return TM_ForcedByUser instead of TM_SuppressedByUser.

asbirlea retitled this revision from Reapply rL352238: [WarnMissedTransforms] Set default to 1. to Check bool attribute value in getOptionalBoolLoopAttribute..Jan 28 2019, 2:33 PM
asbirlea edited the summary of this revision. (Show Details)
Meinersbur accepted this revision.Jan 28 2019, 5:47 PM

Thanks for catching this. Looks like I did not think about calling getZExtValue on ConstantInt, but didn't notive because ConstantInt* converts to bool implicitly.

It would be good if you could add the noise warning example as a test case.

This revision is now accepted and ready to land.Jan 28 2019, 5:47 PM
asbirlea updated this revision to Diff 184175.Jan 29 2019, 2:21 PM

Added a test that show-cases the issue resolved by this patch.
The test is a duplicate of an existing LoopVectorize tests with the metadata set to false.

This revision was automatically updated to reflect the committed changes.