This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] Fixed crash with aliased functions
ClosedPublic

Authored by xbolva00 on Oct 11 2018, 1:47 AM.

Diff Detail

Event Timeline

xbolva00 created this revision.Oct 11 2018, 1:47 AM
xbolva00 updated this revision to Diff 169168.Oct 11 2018, 1:47 AM
  • added test
xbolva00 updated this revision to Diff 169169.Oct 11 2018, 1:49 AM

I just noticed that the same inferLibFuncAttributes(*M->getFunction(...)) pattern also occurs in LoopIdiomRecognize::processLoopStridedStore() in lib/Transforms/Scalar/LoopIdiomRecognize.cpp which is not fixed by this.

Also, there is inferLibFuncAttributes(*CI->getCalledFunction(), *TLI); in isLocallyOpenedFile() of lib/Transforms/Utils/SimplifyLibCalls.cpp which might suffer from a similar problem (although I have not tested that).

Thanks, I will fix other places too

xbolva00 updated this revision to Diff 169230.Oct 11 2018, 9:49 AM
  • updated other places

Ping, ok now?

Sorry for the delay. Yes, as far as I am concerned, this looks good now.

It would be good if you click Accept revision too :)

jbuening accepted this revision.Oct 13 2018, 1:51 AM

Sorry, I am new to Phabricator.

This revision is now accepted and ready to land.Oct 13 2018, 1:51 AM
This revision was automatically updated to reflect the committed changes.

Please make sure your patches are reviewed by a qualified reviewer before you merge.

llvm/trunk/include/llvm/Transforms/Utils/BuildLibCalls.h
31 ↗(On Diff #169564)

Instead of making this function take a possibly-null pointer, could you add an overload that takes a StringRef and calls getFunction itself? It's essentially equivalent, but it would make the intent more clear.

llvm/trunk/test/Transforms/InstCombine/pr39177.ll
1 ↗(On Diff #169564)

Please add a CHECK line to make sure the expected transform actually happens.

Please make sure your patches are reviewed by a qualified reviewer before you merge.

Ok, sorry. Please see D53338.