Page MenuHomePhabricator

john.brawn (John Brawn)
User

Projects

User does not belong to any projects.

User Details

User Since
Feb 12 2015, 7:44 AM (226 w, 4 d)

Recent Activity

Wed, Jun 12

john.brawn accepted D63148: [MIR] Skip hoisting to throwable or return machine basic blocks.

LGTM.

Wed, Jun 12, 4:48 AM · Restricted Project

Tue, Jun 11

john.brawn reopened D56772: [MIR] Add simple PRE pass to MachineCSE.

This is causing incorrect code generation for this piece of IR:

Tue, Jun 11, 4:35 AM · Restricted Project

Wed, May 22

john.brawn committed rG6c49f58a355b: [ARM][AArch64] Fix incorrect handling of alignment in va_arg code generation (authored by john.brawn).
[ARM][AArch64] Fix incorrect handling of alignment in va_arg code generation
Wed, May 22, 4:41 AM
john.brawn committed rL361372: [ARM][AArch64] Fix incorrect handling of alignment in va_arg code generation.
[ARM][AArch64] Fix incorrect handling of alignment in va_arg code generation
Wed, May 22, 4:41 AM
john.brawn committed rC361372: [ARM][AArch64] Fix incorrect handling of alignment in va_arg code generation.
[ARM][AArch64] Fix incorrect handling of alignment in va_arg code generation
Wed, May 22, 4:41 AM
john.brawn closed D62152: [ARM][AArch64] Fix incorrect handling of alignment in va_arg code generation.
Wed, May 22, 4:41 AM · Restricted Project

Tue, May 21

john.brawn added a comment to D62152: [ARM][AArch64] Fix incorrect handling of alignment in va_arg code generation.

Please verify my understanding of the following is correct:

  1. getTypeUnadjustedAlign() is currently only used to compute calling convention alignment for ARM and AArch64.

Yes, the only places it's called are AArch64ABIInfo::classifyArgumentType and ARMABIInfo::classifyArgumentType.

Tue, May 21, 7:22 AM · Restricted Project

Mon, May 20

john.brawn created D62152: [ARM][AArch64] Fix incorrect handling of alignment in va_arg code generation.
Mon, May 20, 9:42 AM · Restricted Project

Mar 8 2019

john.brawn added a comment to D59080: Merge of global constants does not happen when constants have common linkage.

The title and summary of this patch describe a problem, but they don't describe how this patch fixes that problem (which makes it hard to figure out if this patch is correctly fixing the problem). It should be more like "[GlobalMerge] Do XYZ: We have problem ABC, doing XYZ in GlobalMerge fixes it because ...". Also this patch needs tests.

Mar 8 2019, 3:37 AM

Feb 26 2019

john.brawn accepted D58348: [AArch64] Fix for bug 35094 atomicrmw on Armv8.1-A+lse.

LGTM.

Feb 26 2019, 8:00 AM · Restricted Project

Feb 25 2019

john.brawn accepted D58490: [ARM] Be super conservative about atomics.

LGTM.

Feb 25 2019, 10:28 AM · Restricted Project
john.brawn added inline comments to D58490: [ARM] Be super conservative about atomics.
Feb 25 2019, 4:40 AM · Restricted Project

Feb 21 2019

john.brawn added inline comments to D58348: [AArch64] Fix for bug 35094 atomicrmw on Armv8.1-A+lse.
Feb 21 2019, 5:08 AM · Restricted Project

Feb 18 2019

john.brawn requested changes to D57820: [AArch64] Use CAS loops for all atomic operations when available..

It's a bit hard to know whether to accept this or not, given that you says it's "probably better to avoid LDXR". Do you have any data to say if it is any better or not?

Feb 18 2019, 4:33 AM · Restricted Project

Feb 15 2019

john.brawn added a comment to D57820: [AArch64] Use CAS loops for all atomic operations when available..

It's a bit hard to know whether to accept this or not, given that you says it's "probably better to avoid LDXR". Do you have any data to say if it is any better or not?

Feb 15 2019, 10:08 AM · Restricted Project
john.brawn accepted D58166: [BPI] Look through bitcasts in calcZeroHeuristic.

LGTM. Looking at it it does make me wonder if we should handle things that aren't ConstantInt, e.g.

bitcast <2 x i16> <i16 0, i16 0> to i32

is still zero, though I'm guessing things like this actually probably never happen and this commit is fine as it is anyway.

Feb 15 2019, 3:39 AM · Restricted Project

Jan 4 2019

john.brawn committed rL350408: [LICM] Adjust how moving the re-hoist point works.
[LICM] Adjust how moving the re-hoist point works
Jan 4 2019, 9:16 AM
john.brawn closed D55266: [LICM] Use per-block hoist points when rehoisting instructions.
Jan 4 2019, 9:15 AM

Dec 14 2018

john.brawn committed rL349151: [RegAllocGreedy] IMPLICIT_DEF values shouldn't prefer registers.
[RegAllocGreedy] IMPLICIT_DEF values shouldn't prefer registers
Dec 14 2018, 6:11 AM
john.brawn closed D55652: [RegAllocGreedy] IMPLICIT_DEF values shouldn't prefer registers.
Dec 14 2018, 6:11 AM

Dec 13 2018

john.brawn created D55652: [RegAllocGreedy] IMPLICIT_DEF values shouldn't prefer registers.
Dec 13 2018, 6:11 AM

Dec 10 2018

john.brawn created D55512: [MemDep] Adjust NonLocalPointerDep handling to be more precise..
Dec 10 2018, 7:39 AM

Dec 7 2018

john.brawn added inline comments to D53706: [MultiTailCallElimination]: Pass to eliminate multiple tail calls.
Dec 7 2018, 7:54 AM
john.brawn added inline comments to D53706: [MultiTailCallElimination]: Pass to eliminate multiple tail calls.
Dec 7 2018, 5:23 AM

Dec 6 2018

john.brawn updated the diff for D55266: [LICM] Use per-block hoist points when rehoisting instructions.

Adjust according to review comments, and also add some more tests.

Dec 6 2018, 7:56 AM

Dec 5 2018

john.brawn accepted D55009: [GVN] Don't perform scalar PRE on GEPs.

OK, then this looks good to me.

Dec 5 2018, 10:08 AM

Dec 4 2018

john.brawn added inline comments to D55009: [GVN] Don't perform scalar PRE on GEPs.
Dec 4 2018, 8:17 AM
john.brawn added a comment to D55009: [GVN] Don't perform scalar PRE on GEPs.

The idea behind allowing Scalar PRE when the addressing mode isn't legal is for some thing like (adjusted from pre-gep-load.ll)

define void @foo(i32 %stat, i32 %i, double* %p, double* %q) {
entry:
  switch i32 %stat, label %sw.default [
    i32 0, label %sw.bb
    i32 1, label %sw.bb
    i32 2, label %sw.bb2
  ]
Dec 4 2018, 8:09 AM
john.brawn created D55266: [LICM] Use per-block hoist points when rehoisting instructions.
Dec 4 2018, 6:14 AM

Nov 29 2018

john.brawn committed rL347889: [LICM] Reapply r347776 "Make LICM able to hoist phis" with fix.
[LICM] Reapply r347776 "Make LICM able to hoist phis" with fix
Nov 29 2018, 9:13 AM

Nov 28 2018

john.brawn added a comment to D55009: [GVN] Don't perform scalar PRE on GEPs.

It may be worthwhile allowing scalar PRE on GEPs that we know won't be combined into the addressing mode of a load/store, i.e. those where TargetTransformInfo::isLegalAddressingMode returns false.

Nov 28 2018, 10:01 AM
john.brawn committed rL347778: [LICM] Enable control flow hoisting by default.
[LICM] Enable control flow hoisting by default
Nov 28 2018, 9:26 AM
john.brawn closed D54949: [LICM] Enable control flow hoisting by default.
Nov 28 2018, 9:26 AM
john.brawn committed rL347776: [LICM] Reapply r347190 "Make LICM able to hoist phis" with fix.
[LICM] Reapply r347190 "Make LICM able to hoist phis" with fix
Nov 28 2018, 9:24 AM
john.brawn closed D52827: [LICM] Make LICM able to hoist phis.
Nov 28 2018, 9:24 AM
john.brawn accepted D54932: [CGP] Improve compile time for complex addressing mode.

LGTM.

Nov 28 2018, 8:50 AM

Nov 27 2018

john.brawn added a comment to D54932: [CGP] Improve compile time for complex addressing mode.

This looks good, but I think that some of the comments need to be adjusted as they're no longer accurate for what the code is doing.

Nov 27 2018, 7:15 AM
john.brawn added a child revision for D52827: [LICM] Make LICM able to hoist phis: D54949: [LICM] Enable control flow hoisting by default.
Nov 27 2018, 6:31 AM
john.brawn added a parent revision for D54949: [LICM] Enable control flow hoisting by default: D52827: [LICM] Make LICM able to hoist phis.
Nov 27 2018, 6:31 AM
john.brawn created D54949: [LICM] Enable control flow hoisting by default.
Nov 27 2018, 6:30 AM
john.brawn updated the diff for D52827: [LICM] Make LICM able to hoist phis.

Added an option to enable control flow hoisting, turned off by default.

Nov 27 2018, 6:29 AM

Nov 26 2018

john.brawn accepted D54517: [CGP] Limit Complex Addressing mode by number of BasicBlocks to traverse.

Looks OK as a fix for the reported bug, with one minor nitpick. Taking a look at PR39625 it looks like there's an underlying problem where we insert a load of useless placeholders, e.g. for

declare void @otherfn()
Nov 26 2018, 7:40 AM

Nov 22 2018

john.brawn added a comment to D52827: [LICM] Make LICM able to hoist phis.

This rehoisting stuff has been wary for me before, and now I'm completely hesitant about what is going on there. Why do we end up hoisting to some block that does not dominate its users? Do we have a strong reason of doing so rather than finding the correct hoisting destination?

Nov 22 2018, 6:20 AM
john.brawn committed rL347456: [AArch64] Fix SelectionDAG infinite loop for v1i64 SCALAR_TO_VECTOR.
[AArch64] Fix SelectionDAG infinite loop for v1i64 SCALAR_TO_VECTOR
Nov 22 2018, 3:48 AM

Nov 20 2018

john.brawn requested review of D52827: [LICM] Make LICM able to hoist phis.
Nov 20 2018, 9:13 AM
john.brawn reopened D52827: [LICM] Make LICM able to hoist phis.
Nov 20 2018, 9:12 AM
john.brawn updated the diff for D52827: [LICM] Make LICM able to hoist phis.

The thing causing the crash was incorrect handling of instructions that need to be rehoisted. When we have hoisted a phi, then hoisted a use of that phi, then if have to rehoist that use we need to make sure that we rehoist it to _after_ the hoisted phi. We can do this by rehoisting to the immediate dominator instead of just rehoisting everything to the original preheader.

Nov 20 2018, 9:12 AM

Nov 19 2018

john.brawn committed rL347190: [LICM] Make LICM able to hoist phis.
[LICM] Make LICM able to hoist phis
Nov 19 2018, 3:34 AM
john.brawn closed D52827: [LICM] Make LICM able to hoist phis.
Nov 19 2018, 3:34 AM

Nov 14 2018

john.brawn committed rL346869: [SimplifyCFG] Regenerate preserve-branchweights.ll test. NFC.
[SimplifyCFG] Regenerate preserve-branchweights.ll test. NFC
Nov 14 2018, 7:29 AM
john.brawn updated the diff for D52827: [LICM] Make LICM able to hoist phis.

Update according to review comments.

Nov 14 2018, 6:01 AM
john.brawn added a comment to D52827: [LICM] Make LICM able to hoist phis.

The patch fails the following test:

...

Likely some corner case related to conditional branch to the same block for true and false is not handled. Please make sure that it passes before we can proceed with the patch.

Nov 14 2018, 5:47 AM

Nov 13 2018

john.brawn added a comment to D53706: [MultiTailCallElimination]: Pass to eliminate multiple tail calls.

Also, we should be disabling this transformation when optimising for size.

Nov 13 2018, 6:05 AM
john.brawn added a comment to D53706: [MultiTailCallElimination]: Pass to eliminate multiple tail calls.

A few general comments:

Nov 13 2018, 6:00 AM

Nov 8 2018

john.brawn updated the diff for D52827: [LICM] Make LICM able to hoist phis.

Use SmallVector for HoistedInstructions, rename getHoistedBlock, add LI and DT verification, other small changes.

Nov 8 2018, 5:22 AM

Nov 7 2018

john.brawn added inline comments to D52827: [LICM] Make LICM able to hoist phis.
Nov 7 2018, 8:46 AM

Nov 6 2018

john.brawn updated the diff for D52827: [LICM] Make LICM able to hoist phis.

Use LoopBlocksRPO to generate the worklist.

Nov 6 2018, 10:12 AM
john.brawn added a comment to D52827: [LICM] Make LICM able to hoist phis.

I generally like what this patch is doing, but the size of code and its complexity overwhelms me. Is it possible to separate the patch into smaller pieces so that it would be easier to review them in isolation? If yes, I'd appreciate that a lot.

Nov 6 2018, 10:07 AM
john.brawn added inline comments to D52827: [LICM] Make LICM able to hoist phis.
Nov 6 2018, 8:39 AM

Nov 5 2018

john.brawn added a comment to D52550: [ARM] Check for sel intrinsic use in ARM CGP.

Because the ABI says that the GE bits are undefined on entry / return from public interfaces (i.e. non-static functions) I think that it is possible to just look at the current translation unit and make a decision based on that. I'm not sure if just looking at whether the sel intrinsic is used will work though - the GE bits could be read by __arm_rsr("APSR"), or by inline assembly, and maybe there are other situations that I haven't thought of.

Nov 5 2018, 8:20 AM
john.brawn accepted D52653: [CodeGen, AArch64] Combine Interleaved Loads which are not covered by the Vectorizer.

A few errors in comments, but otherwise LGTM so long as you fix those errors before committing.

Nov 5 2018, 5:04 AM

Oct 31 2018

john.brawn added a comment to D52827: [LICM] Make LICM able to hoist phis.

Does this lose debug information? I do very little with optimization passes so I don't know, but it would be nice to keep it in mind.

Oct 31 2018, 9:17 AM
john.brawn updated the diff for D52827: [LICM] Make LICM able to hoist phis.

Update according to review comments.

Oct 31 2018, 9:17 AM
john.brawn added inline comments to D52827: [LICM] Make LICM able to hoist phis.
Oct 31 2018, 8:55 AM

Oct 25 2018

john.brawn committed rL345275: [AArch64] Add EXT patterns for 64-bit EXT of a subvector of a 128-bit vector.
[AArch64] Add EXT patterns for 64-bit EXT of a subvector of a 128-bit vector
Oct 25 2018, 8:34 AM
john.brawn closed D53582: [AArch64] Add EXT patterns for 64-bit EXT of a subvector of a 128-bit vector.
Oct 25 2018, 8:34 AM
john.brawn committed rL345271: [AArch64] Refactor definition of EXT patterns to use a multiclass.
[AArch64] Refactor definition of EXT patterns to use a multiclass
Oct 25 2018, 8:03 AM
john.brawn closed D53580: [AArch64] Refactor definition of EXT patterns to use a multiclass.
Oct 25 2018, 8:03 AM
john.brawn committed rL345270: [AArch64] Do 64-bit vector move of 0 and -1 by extracting from the 128-bit move.
[AArch64] Do 64-bit vector move of 0 and -1 by extracting from the 128-bit move
Oct 25 2018, 7:59 AM
john.brawn closed D53579: [AArch64] Do 64-bit vector move of 0 and -1 by extracting from the 128-bit move.
Oct 25 2018, 7:59 AM

Oct 24 2018

john.brawn added a comment to D52227: LLVM: Expose SimplifyCFGPass (as used in PassBuilder::buildModuleOptimizationPipeline) in PassRegistry.def.

Having some way to generate a pipeline string which matches what opt does by default is definitely a good thing. I think that long-term splitting up simplifycfg is probably the best way, and I think I'm going to have to do some of that myself as part of D50723 (as the current implementation causes a lot of problems for bugpoint as the pass list it deduces doesn't do the same thing as -Owhatever), but doing it in this way for now seems fine.

Oct 24 2018, 10:09 AM

Oct 23 2018

john.brawn added parent revisions for D53582: [AArch64] Add EXT patterns for 64-bit EXT of a subvector of a 128-bit vector: D53579: [AArch64] Do 64-bit vector move of 0 and -1 by extracting from the 128-bit move, D53580: [AArch64] Refactor definition of EXT patterns to use a multiclass.
Oct 23 2018, 8:30 AM
john.brawn added a child revision for D53580: [AArch64] Refactor definition of EXT patterns to use a multiclass: D53582: [AArch64] Add EXT patterns for 64-bit EXT of a subvector of a 128-bit vector.
Oct 23 2018, 8:30 AM
john.brawn added a child revision for D53579: [AArch64] Do 64-bit vector move of 0 and -1 by extracting from the 128-bit move: D53582: [AArch64] Add EXT patterns for 64-bit EXT of a subvector of a 128-bit vector.
Oct 23 2018, 8:30 AM
john.brawn created D53582: [AArch64] Add EXT patterns for 64-bit EXT of a subvector of a 128-bit vector.
Oct 23 2018, 8:29 AM
john.brawn created D53580: [AArch64] Refactor definition of EXT patterns to use a multiclass.
Oct 23 2018, 8:28 AM
john.brawn created D53579: [AArch64] Do 64-bit vector move of 0 and -1 by extracting from the 128-bit move.
Oct 23 2018, 8:23 AM
john.brawn added a comment to D52827: [LICM] Make LICM able to hoist phis.

Ping.

Oct 23 2018, 8:02 AM
john.brawn added inline comments to D52653: [CodeGen, AArch64] Combine Interleaved Loads which are not covered by the Vectorizer.
Oct 23 2018, 7:00 AM

Oct 12 2018

john.brawn added inline comments to D52653: [CodeGen, AArch64] Combine Interleaved Loads which are not covered by the Vectorizer.
Oct 12 2018, 7:04 AM

Oct 11 2018

john.brawn added a comment to D52827: [LICM] Make LICM able to hoist phis.

Starting with only high level design comments....

I think this patch is going about things in slightly the wrong way. As far as I can tell, there are two key components to this: 1) hoisting PHIs w/loop invariant selectors and operands, and 2) hoisting instructions from below conditional branches into conditional blocks inserted in preheader. I think these two can and should be separate into distinct patches.

I'm concerned about (2). This really seems like a form of loop peeling, and a currently unrestricted one without a cost model. I'm not sure I'm convinced this belongs in LICM at all.

(1) could be formulated w/o (2). The easiest way would be to form a select chain using the LIV conditions and LIV operands. This also works for any instruction for which we have a predicated form.

I'd not considered going direct from phi to select when hoisting. It seems like it will restrict the kinds of phis that can be hoisted, but we're probably not hoisting them anyway due to the various TODOs here. I'll look into it.

Oct 11 2018, 8:20 AM
john.brawn updated the diff for D52827: [LICM] Make LICM able to hoist phis.

Update based on review comments, and add some more tests.

Oct 11 2018, 8:09 AM

Oct 10 2018

john.brawn committed rL344139: [llvm-exegesis] Fix function return generation so it doesn't return register 0.
[llvm-exegesis] Fix function return generation so it doesn't return register 0
Oct 10 2018, 6:05 AM
john.brawn closed D53074: [llvm-exegesis] Fix function return generation so it doesn't return register 0.
Oct 10 2018, 6:05 AM
john.brawn created D53074: [llvm-exegesis] Fix function return generation so it doesn't return register 0.
Oct 10 2018, 5:32 AM

Oct 8 2018

john.brawn added a comment to D52827: [LICM] Make LICM able to hoist phis.

Starting with only high level design comments....

I think this patch is going about things in slightly the wrong way. As far as I can tell, there are two key components to this: 1) hoisting PHIs w/loop invariant selectors and operands, and 2) hoisting instructions from below conditional branches into conditional blocks inserted in preheader. I think these two can and should be separate into distinct patches.

I'm concerned about (2). This really seems like a form of loop peeling, and a currently unrestricted one without a cost model. I'm not sure I'm convinced this belongs in LICM at all.

(1) could be formulated w/o (2). The easiest way would be to form a select chain using the LIV conditions and LIV operands. This also works for any instruction for which we have a predicated form.

Oct 8 2018, 10:32 AM

Oct 5 2018

john.brawn added a comment to D52827: [LICM] Make LICM able to hoist phis.

This has no impact by itself that I've been able to see, as LICM typically doesn't see such phis as they will have been converted into selects by the time LICM is run

That's surprising... I would have expected this to show up in some cases. Some branches can't eliminated due to side-effects.

Oct 5 2018, 9:50 AM

Oct 3 2018

john.brawn created D52827: [LICM] Make LICM able to hoist phis.
Oct 3 2018, 5:06 AM

Sep 18 2018

john.brawn committed rL342471: [TargetLowering] Android has sincos functions.
[TargetLowering] Android has sincos functions
Sep 18 2018, 6:23 AM
john.brawn closed D52025: [TargetLowering] Android has sincos functions.
Sep 18 2018, 6:23 AM
john.brawn accepted D51942: [InstCombine] Fold (C/x)>0 into x>0 if possible.

LGTM. I think it may be possible to remove the hasAllowReciprocal check, because the point of that (as I understand it) is to allow converting a divide into a multiply-by-reciprocal in ways that can change the end result and I don't think that the transformation that's being done here will give a different result to the untransformed version, but I'm not certain of that and it's fine to leave it as it is.

Sep 18 2018, 4:54 AM

Sep 13 2018

john.brawn added inline comments to D52025: [TargetLowering] Android has sincos functions.
Sep 13 2018, 5:11 AM
john.brawn created D52025: [TargetLowering] Android has sincos functions.
Sep 13 2018, 4:27 AM

Sep 10 2018

john.brawn committed rL341820: [GVN] Invalidate cached info for values replaced by equality propagation.
[GVN] Invalidate cached info for values replaced by equality propagation
Sep 10 2018, 5:29 AM
john.brawn closed D51218: [GVN] Invalidate cached info for values replaced by equality propagation.
Sep 10 2018, 5:29 AM

Sep 7 2018

john.brawn added a comment to D51637: [AsmPrinter] GlobalAlias with non-zero size and public linkage ends up with wrong size, PR38794.

The reason for the default of not emitting a size (which ends up having the effect of the alias having the same size as the aliasee) is for compatibility with gcc where if you have

Sep 7 2018, 10:31 AM
john.brawn updated the diff for D51218: [GVN] Invalidate cached info for values replaced by equality propagation.

I've adjusted the comment in the test case. I'm pretty sure that the part that the test is testing (that PRE is happening) is correct even if the equality propagation didn't happen, as it's based on knowing that the value of two pointers differs by a nonzero value and not anything to do with the provenance of the pointers.

Sep 7 2018, 8:11 AM

Aug 24 2018

john.brawn committed rL340613: [PhiValues] Use callback value handles to invalidate deleted values.
[PhiValues] Use callback value handles to invalidate deleted values
Aug 24 2018, 8:49 AM
john.brawn created D51218: [GVN] Invalidate cached info for values replaced by equality propagation.
Aug 24 2018, 8:03 AM

Aug 23 2018

john.brawn committed rL340529: [GVN] Invalidate cached info for phis when setting dead predecessors to undef.
[GVN] Invalidate cached info for phis when setting dead predecessors to undef
Aug 23 2018, 5:49 AM