Page MenuHomePhabricator

Refactor setAlreadyUnrolled() and setAlreadyVectorized().

Authored by Meinersbur on Jan 31 2019, 10:36 PM.



Loop::setAlreadyUnrolled() and LoopVectorizeHints::setLoopAlreadyUnrolled() both add loop metadata that stops the same loop from being transformed multiple times. This patch merges both implementations.

In doing so we fix 3 potential issues:

  • setLoopAlreadyUnrolled() keept the llvm.loop.vectorize/interleave.* metadata even though it will not be used anymore. This already caused problems such as Change the behavior to the one of setAlreadyUnrolled which deletes this loop metadata.
  • setAlreadyUnrolled() used to create a new LoopID by calling MDNode::get with nullptr as the first operand, then replacing it by the returned references using replaceOperandWith. It is possible that MDNode::get would instead return an existing node (due to de-duplication) that then gets modified. To avoid, use MDNode::getDistinct.
  • LoopVectorizeHints::matchesHintMetadataName() only compares the suffix of the attribute to set the new value for. That is, when called with "enable", would erase attributes such as "llvm.loop.unroll.enable", "llvm.loop.vectorize.enable" and "llvm.loop.distribute.enable" instead of such the one to replace. Fortunately, function was only called with "isvectorized".

Event Timeline

Meinersbur created this revision.Jan 31 2019, 10:36 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 31 2019, 10:36 PM
Herald added a subscriber: dmgreen. · View Herald Transcript
  • Use a TemporaryMDNode instead of nullptr
etherzhhb accepted this revision.Feb 9 2019, 1:41 PM

LGTM with a suggestion

111–113 ↗(On Diff #186281)

maybe you could use "static ConstantInt *get(LLVMContext &Context, const APInt &V);" in this case you do not need to construct the i32 type explicitly

This revision is now accepted and ready to land.Feb 9 2019, 1:41 PM
Meinersbur marked 2 inline comments as done.
  • Use APInt to construct constant in metadata
  • Undo unnecessary test case changes
This revision was automatically updated to reflect the committed changes.