[PATCH] D104662: [SCEVExpander] Prefer pointer expansion for overflow checks

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

In D104662#2973943 <https://reviews.llvm.org/D104662#2973943>, @mkazantsev wrote:

> In D104662#2871330 <https://reviews.llvm.org/D104662#2871330>, @reames wrote:
>
>> In D104662#2832601 <https://reviews.llvm.org/D104662#2832601>, @mkazantsev wrote:
>>
>>> I don't think that increase of loop size may be justified by the fact that InstCombine will later clean it up. Loop passes may go in one loop pipeline, and passes like unswitch may be pessimized by this.
>>
>> This comment doesn't make sense to me.  What increase in loop size are you referring to?   This change adjusts the type used for a generate test, nothing more.
>
> I was looking at code changes in tests. Maybe it's ok,but formally it's an increase that will pessimize unswitching etc. Do we have reasons to think it will be later undone by other transforms, or will cause no harm?

>From the review description: "Note that there are some test diffs, but a) running them through instcombine helps a ton, and b) there's enough missing obvious transforms on both before and after IR that it's clear this isn't performance sensitive."

If there's a specific test which concerns you, then tell me specifically which one it is so I can take a look.  It's possible I missed something in my initial scan.  Otherwise, I have nothing to add as I clearly though the delta reasonable or I'd not have posted the review.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D104662/new/

https://reviews.llvm.org/D104662