This is an archive of the discontinued LLVM Phabricator instance.

NFC: deduplicate isRepeatedBytePattern from clang to LLVM's isBytewiseValue
ClosedPublic

Authored by jfb on Sep 6 2018, 2:09 PM.

Details

Summary

This code was in CGDecl.cpp and really belongs in LLVM. It happened to have isBytewiseValue which served a very similar purpose but wasn't as powerful as clang's version. Remove the clang version, and augment isBytewiseValue to be as powerful so that clang does the same thing it used to.

LLVM part of this patch: D51751

Diff Detail

Event Timeline

jfb created this revision.Sep 6 2018, 2:09 PM
MatzeB added a comment.Sep 6 2018, 2:20 PM
  • I'm not a good person to review clang changes.
  • I assume the review lacks the changes on the llvm side?
  • On the LLVM side this feels like something for Analysis/ValueTracking.h in fact I already see Value *isBytewiseValue(Value *V); there, maybe that is good enough for your purpose?
jfb added a comment.Sep 6 2018, 2:21 PM
  • I'm not a good person to review clang changes.
  • I assume the review lacks the changes on the llvm side?
  • On the LLVM side this feels like something for Analysis/ValueTracking.h in fact I already see Value *isBytewiseValue(Value *V); there, maybe that is good enough for your purpose?

Indeed, I just noticed the same. I'll try to figure out why it isn't firing where I want it to.

jfb added a comment.Sep 6 2018, 2:24 PM
In D51752#1226465, @jfb wrote:
  • I'm not a good person to review clang changes.
  • I assume the review lacks the changes on the llvm side?
  • On the LLVM side this feels like something for Analysis/ValueTracking.h in fact I already see Value *isBytewiseValue(Value *V); there, maybe that is good enough for your purpose?

Indeed, I just noticed the same. I'll try to figure out why it isn't firing where I want it to.

I think I'll just improve isBytewiseValue to do all the things isRepeatedBytePattern does. I'll update this patch to still delete constantIsRepeatedBytePattern, but use the new and improved isBytewiseValue.

MatzeB added a comment.Sep 6 2018, 2:26 PM
  • I assume the review lacks the changes on the llvm side?

Oops missed the link to the llvm changes in the description... At least in theory you should be able to make a review over all changes in parallel by using subversion or the experimental llvm git monorepo with llvm/utils/git-svn/git-llvm (admittedly haven't tried it myself yet either).

jfb added a comment.Sep 6 2018, 2:28 PM
  • I assume the review lacks the changes on the llvm side?

Oops missed the link to the llvm changes in the description... At least in theory you should be able to make a review over all changes in parallel by using subversion or the experimental llvm git monorepo with llvm/utils/git-svn/git-llvm (admittedly haven't tried it myself yet either).

Haha yeah I'm still using my now years-old workflow. Haven't tried the new fancy ones yet, waiting for the actual move to do so :)

jfb updated this revision to Diff 164481.Sep 7 2018, 11:43 AM
  • Use isBytewiseValue instead.
jfb retitled this revision from NFC: move isRepeatedBytePattern from clang to Constant to NFC: deduplicate isRepeatedBytePattern from clang to LLVM's isBytewiseValue.Sep 7 2018, 11:47 AM
jfb edited the summary of this revision. (Show Details)
MatzeB accepted this revision.Sep 7 2018, 1:14 PM

Looks obvious and LGTM.

This revision is now accepted and ready to land.Sep 7 2018, 1:14 PM
jfb added a comment.Sep 7 2018, 2:42 PM

Will wait on D51751 before committing, otherwise clang tests will start failing (because current isRepeatedBytePattern isn't capable enough).

This revision was automatically updated to reflect the committed changes.