User Details
- User Since
- May 5 2018, 9:37 AM (153 w, 2 d)
Yesterday
Add const qualifier.
I've temporarily reverted this change because it causes a significant compile-time regression, see https://llvm-compile-time-tracker.com/compare.php?from=4b7bad9eaea2233521a94f6b096aaa88dc584e23&to=f4d682d6ce6c5b3a41a0acf297507c82f5c21eef&stat=instructions. I assume that wasn't intentional.
Sun, Apr 11
I believe this can be abandoned now, it's no longer relevant now that LICM runs before LoopRotate, so MSSA is required at that point anyway.
I'm somewhat doubtful that this fold will be beneficial if it requires inserting freeze. Is there some broader context here for which this is intended?
Just FYI, recently work on NewGVN has started again, so that's where the focus will (hopefully) be in the future.
(Just to be sure, I checked that the added tests did fail previously.)
Sat, Apr 10
Reverse ping, I believe @foad's comment regarding the test is still open.
Fri, Apr 9
For reference, this is the IR after unrolling but before simplification for the PhaseOrdering test: https://gist.github.com/nikic/efd3aae8b9282902a418f99e01ff5134 (note that mass of assumes on extractvalues is unproblematic, the issue are the assumes on %.pre.pre).
Taking your example from https://bugs.llvm.org/show_bug.cgi?id=49785#c2, it seems to be scaling with about O(n^5.5) with iteration count, which is beyond all reasonable bounds. I think this needs to be mitigated at a lower level than unrolling.
Possibly MergeAliasResults should preserve the offset when merging two PartialAlias with the same offset?
Thu, Apr 8
Rebase
LGTM
Wed, Apr 7
As you are now asserting that only certain values are allowed, I believe you also need to add a verifier check that checks this. It should not be possible to assert using a valid module.
LGTM, just some suggestions for the test.
@jeroen.dobbelaere I'm not sure on the details, but it may be necessary to pass through just the ptr_provenance information with full restrict and include it in the cache key (while omitting all the other metadata).
@spatel These are important canonicalization transforms, that allow us to treat logical and/or the same way as and/or in many other passes. There is shouldAvoidAbsorbingNotIntoSelect() to guard against infinite loops, and probably it needs to be applied in more places.
Tue, Apr 6
It probably makes sense to land this first in any case, so LGTM.
This looks okay, but I wonder whether we shouldn't be adjusting the getIndexedType() API instead. We have similar code elsewhere, for example in https://github.com/llvm/llvm-project/blob/9c5ebf0358960adf28931569a0c801b56c8008d9/llvm/lib/IR/Constants.cpp#L2415-L2430.
LGTM
I'd suggest to simply switch these to update_tests_checks.py instead. Or is there some reason this is not possible for these tests?
Mon, Apr 5
From someone who isn't very familiar with this code: Why do we need the preference flag in the first place? As indirectbr isn't particularly common, can't we check whether the result is a BlockAddress after the fact?
Sun, Apr 4
This problem isn't really limited to this one function in InstSimplify. Might it make sense to adjust UndefValue to actually represent such constants? We'd have to add a bitmask to it that indicates which values are poison.
Sat, Apr 3
Add release note.
Fri, Apr 2
Thu, Apr 1
Here's a reduced version of @lebedev.ri's example:
As we don't have any default-enabled nosync inference, doesn't this effectively disable this code entirely (for the default pipeline)?
Wed, Mar 31
Unconditionally disabling this transformation affects later pipelines that depend on and/or i1s.
To minimize its impact, this patch conservatively checks whether cond2 is an instruction that
creates a poison or its operand creates a poison.
LGTM
It's not really clear why using the index type for this would be correct. The index type is used for GEP index and casting a pointer to the index type doesn't make a whole lot of sense to me. Do you have any LangRef wording or other usages that would show that this is a sensible thing to do?
I think this should be using Val->stripInBoundsOffsets() rather than modify the getUnderlyingObject() API.
Tue, Mar 30
LGTM
Mon, Mar 29
Sun, Mar 28
Rebase
For reference, here's how the alternative approach of not using xor for zero initialization would be look like (if you ignore some test update failures): https://gist.github.com/nikic/04876ffe96a5ee2ffb4be102cbbb8d60
Sat, Mar 27
LGTM
Fri, Mar 26
Here is a further reduction:
I'm somewhat confused by the implementation approach in this patch. I think there's really two orthogonal cases: The first is if all operations are non-wrapping, in which case we can optimize for arbitrary modulos. The second is that we can always use power of two factors from the GCD, because arithmetic is over a power of two field.
Thu, Mar 25
LGTM, thanks for pulling this through!