Page MenuHomePhabricator

wristow (Warren Ristow)
User

Projects

User does not belong to any projects.

User Details

User Since
Jan 29 2015, 2:13 PM (233 w, 3 d)

Recent Activity

Fri, Jul 19

wristow accepted D64780: Disallow most calling convention attributes on PS4..

LGTM aside from a nit. You should give other reviewers a chance to sign off in case they have additional comments.

Fri, Jul 19, 11:14 AM · Restricted Project, Restricted Project

Wed, Jul 17

wristow added reviewers for D64780: Disallow most calling convention attributes on PS4.: rnk, rjmccall.
Wed, Jul 17, 10:21 AM · Restricted Project, Restricted Project

Mon, Jul 15

wristow added a comment to D64672: [X86] Prevent passing vectors of __int128 as <X x i128> in llvm IR.

Do we need to keep the old behavior on platforms where clang is the de facto compiler?

Mon, Jul 15, 11:06 AM

Jun 17 2019

wristow committed rG6452bdd29b5a: [LV] Suppress vectorization in some nontemporal cases (authored by wristow).
[LV] Suppress vectorization in some nontemporal cases
Jun 17 2019, 10:18 AM
wristow committed rL363581: [LV] Suppress vectorization in some nontemporal cases.
[LV] Suppress vectorization in some nontemporal cases
Jun 17 2019, 10:17 AM
wristow closed D61764: [LV] Suppress vectorization in some nontemporal cases.
Jun 17 2019, 10:16 AM · Restricted Project

Jun 13 2019

wristow added inline comments to D61764: [LV] Suppress vectorization in some nontemporal cases.
Jun 13 2019, 7:37 PM · Restricted Project

Jun 12 2019

wristow updated the diff for D61764: [LV] Suppress vectorization in some nontemporal cases.

Updated patch to address comments from @RKSimon cand @fhahn -- primarily change the default so that nontemporal misaligned mem-ops are assumed to not exist.

Jun 12 2019, 1:46 AM · Restricted Project

Jun 11 2019

wristow added a comment to D61764: [LV] Suppress vectorization in some nontemporal cases.

Thanks @fhahn. I'll update the patch to address those comments.

Jun 11 2019, 2:25 PM · Restricted Project
wristow added a comment to D61764: [LV] Suppress vectorization in some nontemporal cases.

This patch was accepted by @andreadb about a week ago, but Andrea said he'd leave the final decision to @RKSimon. With Simon's comments from a week ago asking @fhahn if he had any preferences for keeping the current default as-is, or changing it to something that seems to reflect existing hardware better, I've been hesitant to actually commit it, even though it's marked as Accepted. With a week now gone by, I'll plan on committing this tomorrow, unless Simon or Florian raise any concerns.

Jun 11 2019, 11:46 AM · Restricted Project

Jun 4 2019

wristow added inline comments to D61764: [LV] Suppress vectorization in some nontemporal cases.
Jun 4 2019, 10:53 AM · Restricted Project

May 31 2019

wristow added inline comments to D61764: [LV] Suppress vectorization in some nontemporal cases.
May 31 2019, 9:25 AM · Restricted Project
wristow added a comment to D61764: [LV] Suppress vectorization in some nontemporal cases.

Would it be possible to add tests where non-temporal load/stores successfully vectorize?

Glad to see your comment about SSE4A supporting nt-stores at any alignment. With that, I can make an X86 test-case that does vectorize.

May 31 2019, 1:05 AM · Restricted Project
wristow updated the diff for D61764: [LV] Suppress vectorization in some nontemporal cases.

Updated the patch to allow arbitrary alignment of float and double nt-stores for SSE4A.

May 31 2019, 12:54 AM · Restricted Project

May 28 2019

wristow added inline comments to D61764: [LV] Suppress vectorization in some nontemporal cases.
May 28 2019, 4:49 PM · Restricted Project
wristow added a comment to D61764: [LV] Suppress vectorization in some nontemporal cases.

Would it be possible to add tests where non-temporal load/stores successfully vectorize?

May 28 2019, 11:16 AM · Restricted Project

May 16 2019

wristow added a comment to D61764: [LV] Suppress vectorization in some nontemporal cases.

ping

May 16 2019, 3:38 PM · Restricted Project

May 9 2019

wristow added a comment to D61764: [LV] Suppress vectorization in some nontemporal cases.

One point I want to explicitly raise, is that prior to this proposed change, the current vectorization implementation ignores the nontemporal hint when checking whether a memory op is vectorizable. So conceptually, it's as though the two new routines:
bool isLegalNTStore(Type *DataType, unsigned Alignment)
bool isLegalNTLoad(Type *DataType, unsigned Alignment)
return true for all types and alignments. With that in mind, I've made the (default) target-independent implementations of these functions return true -- it's only the overriding functions on X86 (which is the target PR40759 was reported against) that will ever return false here (so on non X86 targets, this would be an NFC commit). But in looking through the other backends, I believe that none of them support misaligned nontemporal mem ops. So possibly a better default would be to have these new functions return true only if the specified DataType is minimally aligned at Alignment.

May 9 2019, 3:27 PM · Restricted Project
wristow created D61764: [LV] Suppress vectorization in some nontemporal cases.
May 9 2019, 3:22 PM · Restricted Project

May 8 2019

wristow added a comment to D55232: [SCEV] Suppress hoisting insertion point of binops when unsafe.

lgtm

May 8 2019, 11:50 AM · Restricted Project
wristow committed rGd27b0c624722: [SCEV] Suppress hoisting insertion point of binops when unsafe (authored by wristow).
[SCEV] Suppress hoisting insertion point of binops when unsafe
May 8 2019, 11:48 AM
wristow committed rL360280: [SCEV] Suppress hoisting insertion point of binops when unsafe.
[SCEV] Suppress hoisting insertion point of binops when unsafe
May 8 2019, 11:48 AM
wristow closed D55232: [SCEV] Suppress hoisting insertion point of binops when unsafe.
May 8 2019, 11:48 AM · Restricted Project

May 7 2019

wristow added inline comments to D55232: [SCEV] Suppress hoisting insertion point of binops when unsafe.
May 7 2019, 7:51 PM · Restricted Project
wristow updated the diff for D55232: [SCEV] Suppress hoisting insertion point of binops when unsafe.

Thanks for the review @sanjoy!

May 7 2019, 7:47 PM · Restricted Project

May 6 2019

wristow added a comment to D55232: [SCEV] Suppress hoisting insertion point of binops when unsafe.

ping x 3

May 6 2019, 9:54 AM · Restricted Project

May 1 2019

wristow accepted D61355: [ThinLTO] Fix unreachable code when parsing summary entries..

Seems pretty straightforward and clearly correct.
LGTM

May 1 2019, 9:22 AM · Restricted Project

Apr 24 2019

wristow added a comment to D55232: [SCEV] Suppress hoisting insertion point of binops when unsafe.

ping

Apr 24 2019, 9:57 PM · Restricted Project

Apr 8 2019

wristow added a comment to D55232: [SCEV] Suppress hoisting insertion point of binops when unsafe.

ping

Apr 8 2019, 3:13 PM · Restricted Project

Mar 29 2019

wristow updated the diff for D55232: [SCEV] Suppress hoisting insertion point of binops when unsafe.

Limiting to constants looks super-conservative. How about getting SCEV for RHS and then asking SE isKnownPredicate(ICMP_NE, RHS, 0)?

Sounds good. ... In any case, until the fix that was done as rL347934 is re-instated in some form, I'll hold off on making the fix discussed here.

Mar 29 2019, 4:54 PM · Restricted Project

Mar 18 2019

wristow committed rGad7d0ded2e45: [SCEV] Guard movement of insertion point for loop-invariants (authored by wristow).
[SCEV] Guard movement of insertion point for loop-invariants
Mar 18 2019, 11:52 AM
wristow committed rL356392: [SCEV] Guard movement of insertion point for loop-invariants.
[SCEV] Guard movement of insertion point for loop-invariants
Mar 18 2019, 11:52 AM
wristow closed D57428: [SCEV] Guard movement of insertion point for loop-invariants (take 2).
Mar 18 2019, 11:52 AM · Restricted Project
wristow added a comment to D57428: [SCEV] Guard movement of insertion point for loop-invariants (take 2).

Thanks for the review @sanjoy . I've updated it with a comment about IndVarSimplify, and will commit.

Mar 18 2019, 11:52 AM · Restricted Project

Mar 15 2019

wristow added a comment to D57428: [SCEV] Guard movement of insertion point for loop-invariants (take 2).

Apologies for the late review Warren.

I agree that it is weird that the insert point is a PHI node. Have you traced through the code to see how we're ending up in that situation? Is IndVarSimplify choosing this insertion point?

Mar 15 2019, 2:51 PM · Restricted Project

Mar 14 2019

wristow added a comment to D57428: [SCEV] Guard movement of insertion point for loop-invariants (take 2).

ping x 3

Mar 14 2019, 11:35 AM · Restricted Project

Mar 4 2019

wristow added a comment to D57428: [SCEV] Guard movement of insertion point for loop-invariants (take 2).

ping

Mar 4 2019, 11:32 AM · Restricted Project

Feb 24 2019

wristow added a comment to D57428: [SCEV] Guard movement of insertion point for loop-invariants (take 2).

ping

Feb 24 2019, 9:59 PM · Restricted Project

Feb 19 2019

wristow added a comment to D57428: [SCEV] Guard movement of insertion point for loop-invariants (take 2).

I did not look into this too deeply, but the problem you're describing sounds like it can be solved by setting InsertPt to getFirstNonPHI at some point.

Feb 19 2019, 5:02 PM · Restricted Project

Feb 7 2019

Herald added a project to D57428: [SCEV] Guard movement of insertion point for loop-invariants (take 2): Restricted Project.

ping

Feb 7 2019, 7:03 PM · Restricted Project

Jan 29 2019

wristow updated subscribers of D57428: [SCEV] Guard movement of insertion point for loop-invariants (take 2).
Jan 29 2019, 5:30 PM · Restricted Project
wristow created D57428: [SCEV] Guard movement of insertion point for loop-invariants (take 2).
Jan 29 2019, 5:14 PM · Restricted Project

Dec 5 2018

wristow added inline comments to D55232: [SCEV] Suppress hoisting insertion point of binops when unsafe.
Dec 5 2018, 6:19 PM · Restricted Project
wristow added a comment to D54713: [SCEV] Guard movement of insertion point for loop-invariants.

This fix (rL347934) was reverted at rL348426 because it was causing SEGVs in instcombine. As noted in D55232, a similar fix as this one was originally committed about a year ago (rL289412), but it was reverted shortly thereafter at rL289453, because of OOB PHI operand access in instcombine. The cause of the SEGVs here may be the same underlying issue.

Dec 5 2018, 4:29 PM

Dec 3 2018

wristow updated subscribers of D55232: [SCEV] Suppress hoisting insertion point of binops when unsafe.

Since the callers of SCEVExpander::InsertBinop "know" whether the binop being inserted is safe to hoist, this proposed solution of having the caller pass a parameter to indicate whether it's safe seems clean. But a simpler approach is to special-case the opcode Instruction::UDiv in InsertBinop.

Dec 3 2018, 12:55 PM · Restricted Project
wristow created D55232: [SCEV] Suppress hoisting insertion point of binops when unsafe.
Dec 3 2018, 12:49 PM · Restricted Project

Nov 29 2018

wristow committed rL347934: [SCEV] Guard movement of insertion point for loop-invariants.
[SCEV] Guard movement of insertion point for loop-invariants
Nov 29 2018, 4:06 PM
wristow closed D54713: [SCEV] Guard movement of insertion point for loop-invariants.
Nov 29 2018, 4:06 PM

Nov 28 2018

wristow added a comment to D54713: [SCEV] Guard movement of insertion point for loop-invariants.

I'm not sure whether you use a custom pipeline or not. But my point was that code came to vectorization pass seems strange because trivial loop unswitch optimization (provided by LoopUnswitch pass) could hoist the invariant check. By some reason it did not happen.

Nov 28 2018, 10:46 PM
wristow added a comment to D54713: [SCEV] Guard movement of insertion point for loop-invariants.

@mkazantsev: Thanks for clarifying about the Unit test approach. I'll create a Unit test to check on the non-preheader case as a followup commit.

Nov 28 2018, 10:16 PM
wristow added a comment to D54713: [SCEV] Guard movement of insertion point for loop-invariants.

Thanks for the review. I'll hold off for a day or so before committing, to give Max and Sanjoy an opportunity to object. (And I'll remove the {} that you suggested, before committing.)

Nov 28 2018, 7:10 PM
wristow updated the diff for D54713: [SCEV] Guard movement of insertion point for loop-invariants.

Tabs were accidentally added in the previous updated patch. This update just removes those tabs.

Nov 28 2018, 11:55 AM
wristow updated the diff for D54713: [SCEV] Guard movement of insertion point for loop-invariants.

Have the SafeToHoist check guard the entire hoisting loop, as discussed in the review.

Nov 28 2018, 11:46 AM
wristow added inline comments to D54713: [SCEV] Guard movement of insertion point for loop-invariants.
Nov 28 2018, 9:55 AM

Nov 27 2018

wristow added inline comments to D54713: [SCEV] Guard movement of insertion point for loop-invariants.
Nov 27 2018, 11:20 PM
wristow added inline comments to D54713: [SCEV] Guard movement of insertion point for loop-invariants.
Nov 27 2018, 11:15 PM
wristow added inline comments to D54713: [SCEV] Guard movement of insertion point for loop-invariants.
Nov 27 2018, 11:07 PM

Nov 26 2018

wristow added inline comments to D54713: [SCEV] Guard movement of insertion point for loop-invariants.
Nov 26 2018, 6:23 PM
wristow added inline comments to D54713: [SCEV] Guard movement of insertion point for loop-invariants.
Nov 26 2018, 11:41 AM

Nov 19 2018

wristow created D54713: [SCEV] Guard movement of insertion point for loop-invariants.
Nov 19 2018, 10:39 AM

Oct 11 2018

wristow added a comment to D52836: [LTO] Account for overriding lib calls via the alias attribute.

Thanks for explaining, and sorry for the trouble!
Test updated at r344286.

Oct 11 2018, 1:29 PM
wristow committed rL344286: Update test of r344198 to work with release builds..
Update test of r344198 to work with release builds.
Oct 11 2018, 1:21 PM
wristow added a comment to D52836: [LTO] Account for overriding lib calls via the alias attribute.

However, the function argument uses an implicit label %0, causing failure of LTO/X86/libcall-overridden-via-alias.ll

; Function Attrs: norecurse nounwind readnone
define internal float @logf(float) #2 {
  %2 = fadd float %0, %0
  ret float %2
}
Oct 11 2018, 11:01 AM

Oct 10 2018

wristow committed rL344198: [LTO] Account for overriding lib calls via the alias attribute.
[LTO] Account for overriding lib calls via the alias attribute
Oct 10 2018, 3:56 PM
wristow closed D52836: [LTO] Account for overriding lib calls via the alias attribute.
Oct 10 2018, 3:56 PM

Oct 5 2018

wristow added a comment to D52836: [LTO] Account for overriding lib calls via the alias attribute.

I'll respond more on the bug (especially since I can't reproduce the actual link time error), ...

Thanks for that. I've responded more thoroughly in the bug, but a short note here to clarify.

Oct 5 2018, 6:22 PM

Oct 3 2018

wristow created D52836: [LTO] Account for overriding lib calls via the alias attribute.
Oct 3 2018, 10:57 AM

Sep 21 2018

wristow committed rL342786: [Loop Vectorizer] Abandon vectorization when no integer IV found.
[Loop Vectorizer] Abandon vectorization when no integer IV found
Sep 21 2018, 4:08 PM
wristow closed D52327: [Loop Vectorizer] Abandon vectorization when no integer IV found.
Sep 21 2018, 4:08 PM
wristow updated the diff for D52327: [Loop Vectorizer] Abandon vectorization when no integer IV found.

Added test from D47216.

Sep 21 2018, 3:49 PM
wristow added a comment to D52327: [Loop Vectorizer] Abandon vectorization when no integer IV found.

Regarding the LIT test from D47216, isn't that essentially the same as the LIT test I have here?

I say yes and no. One induction versus two inductions. Given that there were none before, it's nice to have more than one, especially so if another one is handily available. I don't insist, though.

Sep 21 2018, 3:39 PM
wristow updated the diff for D52327: [Loop Vectorizer] Abandon vectorization when no integer IV found.

As the discussion on the possibility of creating an integer IV goes on, I'm updating the patch to include the non-NULL assertion for IdxTy.

Sep 21 2018, 2:38 PM
wristow added a comment to D52327: [Loop Vectorizer] Abandon vectorization when no integer IV found.

Sorry, I totally forgot about my old patch https://reviews.llvm.org/D47216.

  1. I like your two different message version better than changing Line 788 condition to if (!WidestIndTy). That's good.
  2. Please add non-NULL assertion after "Type *IdxTy = Legal->getWidestInductionType();" in InnerLoopVectorizer::getOrCreateTripCount().
  3. Please include LIT test from D47216.
Sep 21 2018, 1:08 PM

Sep 20 2018

wristow created D52327: [Loop Vectorizer] Abandon vectorization when no integer IV found.
Sep 20 2018, 3:36 PM

Jun 18 2018

wristow added inline comments to D48289: refactor of visitFADD for AllowNewConst cases.
Jun 18 2018, 12:31 PM
wristow added inline comments to D48289: refactor of visitFADD for AllowNewConst cases.
Jun 18 2018, 12:28 PM

Jun 7 2018

wristow added a comment to D45968: StackSlotColoring: Decide colors per stack ID.

The concept looks like the right thing, and the code looks good to me. I feel I don't have the experience in this area to officially sign off on it with a "LGTM", but I have no qualms about this after reviewing it.

Jun 7 2018, 5:53 PM

Jun 5 2018

wristow added inline comments to D47749: guard fsqrt with fmf sub flags.
Jun 5 2018, 11:57 AM
wristow added inline comments to D47291: Proposal to make rtti errors more generic..
Jun 5 2018, 11:12 AM

Jun 4 2018

wristow added inline comments to D47291: Proposal to make rtti errors more generic..
Jun 4 2018, 2:44 PM

May 24 2018

wristow added a comment to D47335: [InstCombine] Enable more reassociations using FMF 'reassoc' + 'nsz'.

Thanks for the quick reviews!
Regarding:

Technically, this isn't only an instcombine change.

I left the [InstCombine] tag in the commit message, since this currently is impacting mostly (maybe only) -instcombine. For a moment, I considered [InstCombine,Reassociate], but decided to leave it as-is.

May 24 2018, 1:35 PM
wristow committed rL333221: [InstCombine] Enable more reassociations using FMF 'reassoc' + 'nsz'.
[InstCombine] Enable more reassociations using FMF 'reassoc' + 'nsz'
May 24 2018, 1:20 PM
wristow closed D47335: [InstCombine] Enable more reassociations using FMF 'reassoc' + 'nsz'.
May 24 2018, 1:20 PM
wristow created D47335: [InstCombine] Enable more reassociations using FMF 'reassoc' + 'nsz'.
May 24 2018, 10:19 AM

May 21 2018

wristow abandoned D26708: Fix -f[no-]reciprocal-math -ffast-math interaction, including LTO.

Abandoning this very old proposed patch. This patch was about some fundamental issues with the "umbrella" aspect of the FMF fast. This was intended to be a small step in solving those issues. That umbrella aspect of fast was fixed by by Sanjay (https://reviews.llvm.org/D39304). With that groundwork done, there have been a handful of additional improvements. With all those improvements, the issue of this patch is no longer a problem.

May 21 2018, 3:12 PM

May 17 2018

wristow added a reviewer for D46982: Do not enable RTTI with -fexceptions, for PS4: probinson.
May 17 2018, 9:45 PM

May 2 2018

wristow added a comment to D45710: Fast Math Flag mapping into SDNode.

This is moving closer to being an NFC patch that purely cleans up the FMF bits, which makes sense to me. Ideally, it would be nice if only that clean-up was done in the initial patch, and then a later patch makes changes to DAGCombiner, etc. I think that's what Sanjay is saying. (But I think that because the semantics of the 'UnsafeAlgebra' flag (which is being removed and references being replaced by something else) aren't precisely defined, it may not be a guaranteed NFC patch -- but that's the goal.)

May 2 2018, 11:57 PM
wristow added a comment to D46322: [SelectionDAG] propagate 'afn' and 'reassoc' from IR fast-math-flags.

I realize that you're abandoning this, in favor of simplifying D45710, but there was one point here that I wanted to comment on. Specifically:

May 2 2018, 11:54 PM

May 1 2018

wristow added a comment to D45022: [X86] Mark all byval parameters as aliased.

A couple minor test-tweak comments in-line.
As before, the code-change looks safe and conservative to me, in terms of the aliasing. And my guess is that conservative aspect is fine, in that it won't seriously impact performance. So I'm happy to say LGTM. Does anyone with more experience in this area have any concerns?

May 1 2018, 5:40 PM

Apr 24 2018

wristow added inline comments to D45022: [X86] Mark all byval parameters as aliased.
Apr 24 2018, 6:52 PM
wristow committed rL330778: [X86] Account for partial stack slot spills (PR30821).
[X86] Account for partial stack slot spills (PR30821)
Apr 24 2018, 3:05 PM
wristow closed D44782: Account for partial stack slot spills (PR30821).
Apr 24 2018, 3:05 PM

Apr 20 2018

wristow added inline comments to D45710: Fast Math Flag mapping into SDNode.
Apr 20 2018, 2:54 AM

Apr 14 2018

wristow added a comment to D45453: [InstCombine] Enable Add/Sub simplifications with only 'reassoc' FMF.

The fsub needs FMF, but not the fmul. That's the general rule we're using in FMF-enabled folds. The non-strict final value (the result of the fsub) is what we're reassociating/factoring here, so as long as that instruction has reassoc+nsz, it doesn't matter if the fmul intermediate value is strict

Apr 14 2018, 7:24 PM
wristow added inline comments to D45453: [InstCombine] Enable Add/Sub simplifications with only 'reassoc' FMF.
Apr 14 2018, 12:53 PM
wristow closed D45453: [InstCombine] Enable Add/Sub simplifications with only 'reassoc' FMF.

Thanks for the review Sanjay. Now committed, at r330089

Apr 14 2018, 12:41 PM
wristow committed rL330089: [InstCombine] Enable Add/Sub simplifications with only 'reassoc' FMF.
[InstCombine] Enable Add/Sub simplifications with only 'reassoc' FMF
Apr 14 2018, 12:22 PM

Apr 13 2018

wristow added inline comments to D45453: [InstCombine] Enable Add/Sub simplifications with only 'reassoc' FMF.
Apr 13 2018, 6:22 PM
wristow updated the diff for D45453: [InstCombine] Enable Add/Sub simplifications with only 'reassoc' FMF.

Thanks Sanjay. Updated the tests to be more careful about when 'nsz' is and isn't needed, along with comments to clarify. Also simplified some tests where no FMF is needed (irrespective of this patch).

Apr 13 2018, 6:12 PM

Apr 12 2018

wristow updated the diff for D45453: [InstCombine] Enable Add/Sub simplifications with only 'reassoc' FMF.

I've updated the code-change in "InstCombineAddSub.cpp" to require both 'reassoc' and 'nsz' to do the transformations. (I haven't changed the name from FAddCombine to FAddSubCombine or ReassociateFAddFSub. I could do that if we move forward with this.)

Apr 12 2018, 10:44 PM