GitHub: https://github.com/nikic
Blog: https://www.npopov.com
User Details
- User Since
- May 5 2018, 9:37 AM (255 w, 3 d)
Sun, Mar 26
Adding some people who might have opinions on more FP intrinsics.
Sat, Mar 25
isSized() should return true for these structs. They are sized for our definition of sized.
Fri, Mar 24
I think this (removing GlobalCleanupPM) makes sense at a high level, but this is a pretty substantial pipeline change and will need more thorough evaluation (e.g. looking at IR diffs on test suite). For example, it is not at all obvious to me that moving InstCombine before EarlyCSE is harmless, given the impact this may have on one-use heuristics.
I've put up https://reviews.llvm.org/D146813 to move the loop invariant GEP reassociation into LICM, which should allow us to drop the LoopInfo dependency from InstCombine.
@mkazantsev Thanks for the context! In that case I'll go with this patch, as it seems like we are already disallowing this everywhere apart from this one fold, so it makes more sense to be consistent.
Minimal test case:
define internal ptr @callee(ptr %dead) { ret ptr null }
LGTM
LGTM
Thu, Mar 23
@nickdesaulniers I don't think we want to handle that on the LLVM side. That will be fundamentally unreliable. If Clang wishes to make "recoverable" ubsan have arbitrary but well-defined behavior in the case of UB, it needs to generate appropriate IR to model that. For example, instead of generating
if (x == 0) { report_div_by_zero(); } res = y / x;
it needs to generate
if (x == 0) { report_div_by_zero(); res = 0; // or any other value } else { res = y / x; }
And similarly for other cases.
I think this is right, but I'm not very confident about it. The problem is that mergeicmps can reorder loads, but I think these isolated comparisons will not get reordered before other ones (only possibly after), so I think it's fine. But maybe I just didn't manage to construct the right test case.
LGTM
LGTM
LG
LGTM
Wed, Mar 22
Thanks for the PhaseOrdering test, I think I get the problem now. ArgPromotion is the first pass in the CGSCC pipeline, and works a bit unusually in that promotion will insert loads in the caller (outside the current SCC). This means the we run ArgPromotion on a child function, which inserts instructions in the parent and simplify the child. Then we run ArgPromotion on the parent, but at this point the parent has not been simplified yet. As such, it is important for ArgPromotion to clean up instructions it inserts. There's possibly some phase ordering improvement we could make here, but I don't see anything obvious. (Just moving ArgPromotion to the end of the CGSCC pipeline would cause the reverse issue that function simplification can not make use of the promotion for cleanup.)
This looks good to me, but could you please pre-commit the new tests with baseline check lines, and then rebase this patch so that only the test diff is visible?
LGTM
LGTM
As @RKSimon said, this transform is in the wrong place. It has nothing to do with shuffled intrinsic operands...
@jdoerfert Yeah, there is a pending patch that fixes this for !range here: https://reviews.llvm.org/D142687 We'll need similar changes for nonnull/align as well.
Could you please also add tests for the !noundef case? (Preferably one with the first load noundef and one with the second. I'm not sure you're checking the right one currently.)
Implementation looks fine, but this needs more test coverage.
LGTM
LGTM
LGTM
Tue, Mar 21
Nearly there, I think...
All of the test changes are, to my eye, semantically the same.
LGTM
LGTM
LGTM
It looks like this broke the expensive checks build (x86-64-baseptr.ll and i386-baseptr.ll fail).
Upload correct patch.
Improve comments.
LGTM
Can you please rebase over https://github.com/llvm/llvm-project/commit/d0de2c51c9a9fc0fedb97ee98f61ce08cb34972b? I hope that will clarify what the actual change here is, because making foldOperationIntoSelectOperand() fallible doesn't make a whole lot of sense.
LGTM
Mon, Mar 20
LGTM
LGTM. Assuming you don't have commit access, can you tell me the Name <email> to use for the commit?
FWIW, I am quite unhappy with the implementation quality of this pass, but I don't think I have the energy to deal with this. In the future, due diligence for pass enablement needs to include a review of the pass implementation by a domain expert, if this was not already done as part of the initial implementation. (Domain expert = SCEV reviewer in this context.)
Using max exit count is incorrect. The computed max may be larger than the actual trip count of this exit.
Change looks fine, but is missing a test case.
ping ;)
Sun, Mar 19
I haven't looked in detail what you're doing here, but you're clearly looking for the ConstantRange class. makeExactICmpRegion(), add(), sub(), binaryNot(), etc.
LG
Extracting the stride from GEPs turned out to be trickier than I expected...