This is an archive of the discontinued LLVM Phabricator instance.

[NFC] Change setDebugLocFromInst to use the class Builder by default
ClosedPublic

Authored by david-arm on Jun 29 2021, 3:40 AM.

Details

Summary

In lots of places we were calling setDebugLocFromInst and passing
in the Builder member variable found in InnerLoopVectorizer. It
seemed neater to modify the existing function to take an Optional
IRBuilder argument that defaults to None, where None means use
the class Builder.

Diff Detail

Event Timeline

david-arm created this revision.Jun 29 2021, 3:40 AM
david-arm requested review of this revision.Jun 29 2021, 3:40 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 29 2021, 3:40 AM
sdesmalen added inline comments.Jun 29 2021, 5:02 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
550–553

Is this interface still needed? I expected that the implementation could now use its Builder member variable.

3040

unrelated whitespace change?

david-arm updated this revision to Diff 355511.Jun 30 2021, 5:18 AM
david-arm retitled this revision from [NFC] Add new setDebugLocFromInst that uses the class Builder to [NFC] Add new setDebugLocFromInst that uses the class Builder by default.
  • Removed the unnecessary extra interface and replaced the existing one to take an Optional instead.
sdesmalen added inline comments.Jun 30 2021, 5:49 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
552

If you make this the second operand, you can make it a default parameter, e.g. Optional<IRBuilder<> *> Ptr = None, so you don't need to pass None to each of the calls.

david-arm updated this revision to Diff 355557.Jun 30 2021, 8:11 AM
  • Used @sdesmalen 's suggestion and swapped the operands around and made the IRBuilder<> option take a default of None.

Thanks for the changes. Can you update the title and commit message (which are now out of sync with the updated patch)?
I also left a nit, but otherwise the patch looks fine to me.

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
552

nit: Can you rename Ptr to CustomIRBuilder or something?

david-arm retitled this revision from [NFC] Add new setDebugLocFromInst that uses the class Builder by default to [NFC] Change setDebugLocFromInst to use the class Builder by default.Jul 1 2021, 1:00 AM
david-arm edited the summary of this revision. (Show Details)
sdesmalen accepted this revision.Jul 1 2021, 4:55 AM

LGTM if you can address my nit about renaming the operand before committing. Thanks for the changes.

This revision is now accepted and ready to land.Jul 1 2021, 4:55 AM