- User Since
- May 5 2018, 9:37 AM (163 w, 2 d)
Any thoughts on the approach here? Is the !isOpaque() ? getElementType() : nullptr pattern something we should add a helper function for? Not sure how common this is going to be outside these cast transforms.
@efriedma LICM still uses AST for scalar promotion.
LGTM, but please drop the "use poison instead of undef" comments -- I don't think using poison instead of undef for dead code requires any explicit justification.
Sat, Jun 19
Remove outdated comment.
Remove obsolete unroll count clamp.
I have a general policy question here: Do we care about miscompiles that are caused by undef, but not poison? This makes a difference for transforms like these, e.g. no freeze is required if the condition is first in a logical or (or at any position in bitwise or) if we only care about poison.
Rebase, and only consider TripCount for all exits, leave TripMultiple alone for now to minimize impact.
No longer needed, UnrollLoop() is no longer passed TripCount/TripMultiple for a specific exit.
Fri, Jun 18
What do you think about this variant? It prints information for all exits as debug output, but keeps optimization remarks basic.
Not seeing any compile-time impact, and rebased versions still looks good.
Compile-time with the reference invalidation issue fixed: https://llvm-compile-time-tracker.com/compare.php?from=b5c4fc0f232b6368bbd7cd8681a6931f2c30ad02&to=9bb9b602fd58e6fdd353cdfedec56980a40230c1&stat=instructions Impact is small and without outliers. I think this is as cheap as it gets :)
Thu, Jun 17
Still crashing, here's the backtrace:
ld: /home/nikic/llvm-project/llvm/include/llvm/Support/Casting.h:104: static bool llvm::isa_impl_cl<To, const From*>::doit(const From*) [with To = llvm::Constant; From = llvm::Value]: Assertion `Val && "isa<> used on a null pointer"' failed.
Fix comment typos.
It looks like the new implementation causes a segfault while building 7zip using the ReleaseLTO-g configuration: https://llvm-compile-time-tracker.com/show_error.php?commit=77e65ca9acb9db4de64a1ff8002f1d53470a0de3
Wed, Jun 16
As a drive-by note, it would be great if you could expand LangRef on non-integral pointers a bit. It made sense to me when it specified that you can't use ptrtoint on a non-integral pointer, but without that limitation, it's not really clear to me what the actual difference between a non-integral pointer and a normal one is. What transforms are you not allowed to perform on a non-integral pointer that you can perform on a normal one?
Tue, Jun 15
Mon, Jun 14
@bjope This SimplifyCFG patch allows us to remove the assume dropping code in CGP subsequently.
Sun, Jun 13
Sat, Jun 12
Fri, Jun 11
Looks like phase ordering tests need an update.
There's a minor compile-time impact for this refactoring: https://llvm-compile-time-tracker.com/compare.php?from=57006d2f6d96d8a6836ae901218ed615071b3b8e&to=eca6ad2e31d754420fb114872bfc4060a698155e&stat=instructions It's okay, but you might want to cut down on the number of repeated/redundant getNotSCEV operations you perform. (They're not cached and known to be somewhat expensive.)
It seems like the addition of this patch has been somewhat botched by adding it only to the NewPM pipeline -- for no clear reason I can discern. This was at a time where barely anyone (outside Google) was using the NewPM, which means that this received very little evaluation from other parties. Now people are hitting this as part of the LegacyPM -> NewPM migration instead, which really shouldn't be the case.
Thu, Jun 10
Wed, Jun 9
It looks like a different fix is being pursued, so marking this as changes requested to move it off the review queue.
While it may make sense to address this in JumpThreading in addition, I believe InstSimplify is intended to be robust against unreachable code. As InstSimplify can thread over phis, I don't think it's even possible for a caller to reliably prevent unreachable code from reaching InstSimplify.
Per above comment
Tue, Jun 8
Okay, I think the change is not right after all. In particular, if we have an exit count like ((1 + %len) /u 2) that does represent the right exit count in arbitrary precision (via either exit or executed UB), but the expression for calculating the exit count itself may still overflow. If %len is UINT_MAX then the resulting exit count would be 0, while it should be UINT_MAX/2 (which is the iteration at which UB occurs). That's clearly not right.
Mon, Jun 7
After looking through the tests, why does this need to use SCEV at all? As far as I can tell, simply using InstSimplify would be sufficient to prove all these cases (or just ConstFold for that matter). Without isKnownPredicateAt(), where symbolic reasoning is necessary, SCEV doesn't seem to provide much value here, or at least it's not obvious from the tests. (Of course, there will be cases where it can do more, but the converse also holds -- in particular, InstSimplify/ConstFold support arbitrary instructions, no need to special-case a limited set of them.)
LGTM. I was going to suggest that you add a direct AA test, but apparently there is no way to write an empty phi in textual LLVM IR, at least not that I can see. Weird limitation.