- User Since
- Nov 16 2012, 6:02 PM (322 w, 1 d)
Thu, Jan 17
Please, in the future, make sure you post full-context patches.
Can we run EarlyCSE after CGP (instead of trying to do these kinds of point fixes)?
Fri, Jan 11
Thu, Jan 10
Wed, Jan 9
Mon, Jan 7
Fri, Jan 4
Wed, Jan 2
Tue, Jan 1
Mon, Dec 31
Sun, Dec 30
Sat, Dec 29
So this rell-request
Fri, Dec 28
Thu, Dec 27
Please fixup the comments, but LGTM.
Fri, Dec 21
I can certainly split this into more patches if that is preferred. I'm unsure though how to test the metadata and abstract call sites without a user.
A high level strategy comment. I think you're trying to go a bit too far in one patch. I'd advise first writing a patch which does nothing but implement the IPConstProp adjustments "the hard way" (i.e. without the AbstractCallSite) and focus on defining the metadata with the semantics you once. To really justify the complexity of the abstraction, you need more than one user. You could add another user to this patch, but doing so will just slow down the review. Separating this into a series of patches (IPConstProp w/metadata def, second user, then introduce abstraction) will make it much easier to see the necessity and make each piece more easily reviewable.
LGTM. This certainly looks like a better solution; the previous code was indeed trying to make sure that the containing CR field was defined before the mfocrf. Thanks!
Dec 20 2018
Thanks for updating this patch. It will be very useful!
Dec 19 2018
Aside from the requested renaming (see below), this LGTM.
Dec 18 2018
Minor typo noted below, but otherwise, LGTM (to avoid any misunderstanding: this should be committed after the LLVM change lands).
Yes, volatile loads and stores access the "same" state as inaccessiblememonly functions, I think.
Also, cost model changes can be tested directly. See tests in test/Analysis/CostModel/PowerPC/
Dec 17 2018
LGTM. Thanks, I think this naming is more accurate and useful.
Such explicit transformations should take precedence over disabling heuristics.
Dec 14 2018
I think that this is okay. There's no semantic change nor a change in control dependence. For this to cause a problem, there would need to be metadata that would change meaning or validity when the instruction changes from an invoke to a call (where the invoke is semantically equivalent to the call).
Dec 13 2018
Care to update the title of this review? ;)
Dec 10 2018
Dec 7 2018
Dec 6 2018
@hfinkel could you please have a look since the last change is related to your part.
This LGTM. However, please, in llvm/Analysis/DemandedBits.h, document what this means for vectors (that it treats all elements the same and returns a mask of the scalar size). You might expand on the comment:
Dec 5 2018
Dec 4 2018
Dec 3 2018
A few additional comments. Otherwise, this LGTM. When @dmgreen is happy with the unrolling changes, I think you're good to go.
Dec 1 2018
Nov 29 2018
Nov 28 2018
Can you please upload the patch with full context?
Nov 26 2018
Nov 22 2018
Nov 21 2018
Nov 20 2018
Nov 19 2018
If not, we can go with the #pragma clang declare simd directive, with clang being ifdef guarded to become omp when _OPENMP is detected.
Nov 18 2018
Nov 16 2018
Just because FENV_ACCESS can be toggled on that granularity doesn't mean we have to represent it that way. We've previously agreed (I think) that if FENV_ACCESS is enabled anywhere in a function we will want to use the constrained intrinsics for all FP operations in the function, not just the ones in the scope where it was specified.
As discussed on IRC, check sign/power of 2 correctly for the alignment assumptions.