dsanders (Daniel Sanders)
User

Projects

User does not belong to any projects.

User Details

User Since
Aug 19 2013, 3:30 PM (252 w, 1 h)

Recent Activity

Fri, Jun 15

dsanders committed rL334871: [globalisel][tablegen] Add support for C++ predicates on PatFrags and use it to….
[globalisel][tablegen] Add support for C++ predicates on PatFrags and use it to…
Fri, Jun 15, 4:18 PM
dsanders accepted D48197: Avoid copying PrettyStackTrace messages an extra time on Apple platforms.

LGTM :-)

Fri, Jun 15, 8:35 AM

Fri, Jun 8

dsanders committed rL334337: [tablegen] Improve performance on *GenRegisterInfo.inc by replacing….
[tablegen] Improve performance on *GenRegisterInfo.inc by replacing…
Fri, Jun 8, 4:16 PM
dsanders closed D47907: [tablegen] Improve performance on *GenRegisterInfo.inc by replacing SparseVector with BitVector. NFC.
Fri, Jun 8, 4:16 PM

Thu, Jun 7

dsanders created D47907: [tablegen] Improve performance on *GenRegisterInfo.inc by replacing SparseVector with BitVector. NFC.
Thu, Jun 7, 2:27 PM
dsanders added inline comments to D45543: [globalisel] Add a combiner helpers for extending loads and use them in a pre-legalize combiner for AArch64.
Thu, Jun 7, 7:59 AM

Wed, May 30

dsanders added inline comments to D47525: [TableGen] Make DAGInstruction own Pattern to avoid leaking it..
Wed, May 30, 9:24 AM
dsanders accepted D47528: [Release Notes] Add release note for the new LLVM_DEBUG.

LGTM with a note about clang-format

Wed, May 30, 8:29 AM
dsanders accepted D47463: [TableGen] Avoid leaking TreePatternNodes by using shared_ptr..

LGTM

Wed, May 30, 8:11 AM
dsanders added inline comments to D47514: [Target][NFC] Simplify `AssemblerPredicate`..
Wed, May 30, 8:10 AM

Tue, May 29

dsanders added inline comments to D47425: [AArch64][GlobalISel] Zero-extend s1 values when returning..
Tue, May 29, 3:07 PM
dsanders added inline comments to D47425: [AArch64][GlobalISel] Zero-extend s1 values when returning..
Tue, May 29, 2:35 PM
dsanders updated the diff for D45543: [globalisel] Add a combiner helpers for extending loads and use them in a pre-legalize combiner for AArch64.

The full patch. The previous update was taken from the wrong machine and hadn't
been finished. Aside from not compiling, there was also a bug where additional
combine attempts would see G_SEXTLOAD mutate into G_LOAD (the anyext form).

Tue, May 29, 12:21 PM
dsanders updated the diff for D45543: [globalisel] Add a combiner helpers for extending loads and use them in a pre-legalize combiner for AArch64.

Rewrite the patch such that the extends hoist up to the loads. The previous
patch had a couple problems that became clear on the bots:

  • The loads could be duplicated. This is a correctness problem for volatile loads but more generally is likely to harm performance.
  • The loads would sink to the extends without any hazard checking
Tue, May 29, 10:42 AM
dsanders reopened D45543: [globalisel] Add a combiner helpers for extending loads and use them in a pre-legalize combiner for AArch64.

Re-opening as I'm about to update it with a revised version. The previous one broke several bots because it sank loads down to the extends

Tue, May 29, 10:17 AM
dsanders added a comment to D47463: [TableGen] Avoid leaking TreePatternNodes by using shared_ptr..

Generally LGTM. I have a couple questions on this one though

Tue, May 29, 8:21 AM
dsanders accepted D47461: [TableGen] Fix leaking of PhysRegInputs..

LGTM

Tue, May 29, 8:06 AM
dsanders accepted D47462: [TableGen] Fix leaking synthesized registers..

LGTM

Tue, May 29, 8:04 AM

May 8 2018

dsanders committed rL331846: Revert r331816 and r331820 - [globalisel] Add a combiner helpers for extending….
Revert r331816 and r331820 - [globalisel] Add a combiner helpers for extending…
May 8 2018, 10:04 PM
dsanders added a comment to D46620: Fix side effect in debug code.

Having the side-effect inside the assert is harmless since the #ifndef ensures the whole loop is only built when asserts are enabled and Index is only used by the assert. However, I agree it's weird to have side-effects inside the assert. Moving the increment out of the assert and into the for loop would LGTM

May 8 2018, 9:11 PM
dsanders added a comment to D46414: [GlobalISel][Legalizer] More concise and faster widenScalar, NFC.

Yes, I do have my doubts that the original implementation with sign extension is always correct, but it's NFC here anyway. I'm thinking to take a look at it at some point and see if I can come up with a breaking test case for that sign extend.

May 8 2018, 4:57 PM
dsanders committed rL331820: [globalisel] Correct r331816 to check the opcode before calling getOperand()..
[globalisel] Correct r331816 to check the opcode before calling getOperand().
May 8 2018, 4:02 PM
dsanders committed rL331816: [globalisel] Add a combiner helpers for extending loads and use them in a pre….
[globalisel] Add a combiner helpers for extending loads and use them in a pre…
May 8 2018, 3:30 PM
dsanders closed D45543: [globalisel] Add a combiner helpers for extending loads and use them in a pre-legalize combiner for AArch64.
May 8 2018, 3:30 PM
dsanders updated the diff for D45543: [globalisel] Add a combiner helpers for extending loads and use them in a pre-legalize combiner for AArch64.

Update before commit

May 8 2018, 3:30 PM
dsanders added inline comments to D45543: [globalisel] Add a combiner helpers for extending loads and use them in a pre-legalize combiner for AArch64.
May 8 2018, 12:05 PM
dsanders added a comment to D45541: [globalisel] Update GlobalISel emitter to match new representation of extending loads.

I can see some optimizations for G_SEXTLOAD/G_ZEXTLOAD since they're only legal when they're an extend so they could just drop the check in the optimized table.

May 8 2018, 9:24 AM
dsanders added a comment to D45541: [globalisel] Update GlobalISel emitter to match new representation of extending loads.

I think we lost 0.5% on compile time on CTMark because of this. Is there any scope left here for performance improvements? With the addition of the combiner landing later, I expect a further CT hit so anything we can do here would be good.

May 8 2018, 8:57 AM

May 5 2018

dsanders committed rL331603: [globalisel] Remove redundant -global-isel option from tests that use -run-pass..
[globalisel] Remove redundant -global-isel option from tests that use -run-pass.
May 5 2018, 2:24 PM
dsanders committed rL331601: [globalisel] Update GlobalISel emitter to match new representation of extending….
[globalisel] Update GlobalISel emitter to match new representation of extending…
May 5 2018, 1:58 PM
dsanders closed D45541: [globalisel] Update GlobalISel emitter to match new representation of extending loads.
May 5 2018, 1:58 PM

May 2 2018

dsanders committed rL331381: [reassociate] Fix excessive revisits when processing long chains of….
[reassociate] Fix excessive revisits when processing long chains of…
May 2 2018, 11:02 AM
dsanders closed D45734: [reassociate] Fix excessive revisits when processing long chains of reassociatable instructions..
May 2 2018, 11:02 AM
dsanders updated the summary of D45734: [reassociate] Fix excessive revisits when processing long chains of reassociatable instructions..
May 2 2018, 11:02 AM
dsanders added inline comments to D45734: [reassociate] Fix excessive revisits when processing long chains of reassociatable instructions..
May 2 2018, 10:28 AM
dsanders added a comment to D45734: [reassociate] Fix excessive revisits when processing long chains of reassociatable instructions..

Thanks

May 2 2018, 9:48 AM
Herald added a reviewer for D45734: [reassociate] Fix excessive revisits when processing long chains of reassociatable instructions.: javed.absar.

I have some performance figures from the LLVM test-suite now using the --benchmarking-only option. Here's the summaries reported by lnt:

metricgeomean before patchgeomean after patchdelta
compile time0.19560.1261-35.54%
execution time0.32400.3237-
code size7365.44597365.6079-
May 2 2018, 9:26 AM

May 1 2018

dsanders added a comment to D45627: [MIPS GlobalISel] Add lowerCall.

I've only gone through the ABI side of things (and from memory) but I have a couple comments.

May 1 2018, 12:39 PM
dsanders accepted D46098: [GlobalISel][InstructionSelect] Making Coverage Info generation optional on per-match table basis.

LGTM

May 1 2018, 12:32 PM
dsanders added a comment to D45990: GlobalISel/InstructionSelector: Implement GIR_CopyFConstantAsFPImm.

I actually don't have a test case of this. I really just need the enum defined so AMDGPUGenGlobalIsel.inc will build. I can drop the implementation and just add the enum value if you prefer.

Could you send me the comment that's shortly before the GIR_CopyFConstantAsFPImm in AMDGPUGenGlobalIsel.inc? It should look similar to a tablegen pattern and we can probably derive a test case from that. I'm not sure which rule has been imported but I'm hoping it will be something like (set $rd:GPR, $imm:fpimm) in which case the test would be similar to test/CodeGen/AArch64/GlobalISel/select-constant.mir.

Here is the comment. Note that the reason I don't have a test case is because this pattern is only for the R600 sub-target and I'm not implementing global-isel for that sub-target:

// MIs[0] Operand 1
// No operand predicates
// (fpimm:{ *:[f32] }):$val  =>  (MOV_IMM_F32:{ *:[f32] } (fpimm:{ *:[f32] }):$val)
GIR_BuildMI, /*InsnID*/0, /*Opcode*/AMDGPU::MOV_IMM_F32,
GIR_Copy, /*NewInsnID*/0, /*OldInsnID*/0, /*OpIdx*/0, // dst
GIR_CopyFConstantAsFPImm, /*NewInsnID*/0, /*OldInsnID*/0, // va
May 1 2018, 11:51 AM
dsanders added a comment to D46096: [GlobalISel][InstructionSelect] Refactoring out a getMatchTable virtual method, NFC.

Thanks. I'd suggest making it a little more obvious to someone skim-reading the list, for example:

[GlobalISel][InstructionSelect] Refactoring out a getMatchTable virtual method + other small NFC's

but LGTM either way.

May 1 2018, 11:44 AM
dsanders accepted D46097: [GlobalISel][InstructionSelect] Refactoring buildMatchTable out, NFC.

LGTM

May 1 2018, 11:20 AM
dsanders accepted D46096: [GlobalISel][InstructionSelect] Refactoring out a getMatchTable virtual method, NFC.

The NFC changes look good to me but the commit message (particularly the subject line) doesn't really match the content. LGTM with a better commit message.

May 1 2018, 11:16 AM
dsanders added a comment to D45990: GlobalISel/InstructionSelector: Implement GIR_CopyFConstantAsFPImm.

I actually don't have a test case of this. I really just need the enum defined so AMDGPUGenGlobalIsel.inc will build. I can drop the implementation and just add the enum value if you prefer.

May 1 2018, 11:08 AM
dsanders accepted D45990: GlobalISel/InstructionSelector: Implement GIR_CopyFConstantAsFPImm.

LGTM. Preferably with a test case in this patch but adding the test cases along with the AMDGPU patches you're working on will also be ok

May 1 2018, 10:51 AM
dsanders added inline comments to D45543: [globalisel] Add a combiner helpers for extending loads and use them in a pre-legalize combiner for AArch64.
May 1 2018, 9:58 AM

Apr 30 2018

dsanders added a comment to D44704: [GlobalISel][X86][ARM] Relaxing type constraints on G_SHL and friends.

It's worth mentioning that:

%1:_(s64) = G_INTTOPTR %0(<4 x s16>)

isn't a valid G_INTTOPTR. The result should be a pointer type and the input should be a scalar. However, your point stands even with that fixed.

Apr 30 2018, 3:33 PM
dsanders committed rL331201: Fix infinite loop after r331115.
Fix infinite loop after r331115
Apr 30 2018, 10:23 AM

Apr 28 2018

dsanders updated the diff for D45541: [globalisel] Update GlobalISel emitter to match new representation of extending loads.

Refresh patch
Fix most of the nits
Update GlobalISelEmitter.td

Apr 28 2018, 8:20 PM
dsanders added inline comments to D45541: [globalisel] Update GlobalISel emitter to match new representation of extending loads.
Apr 28 2018, 5:37 PM
dsanders committed rL331115: [globalisel][legalizerinfo] Introduce dedicated extending loads and add….
[globalisel][legalizerinfo] Introduce dedicated extending loads and add…
Apr 28 2018, 11:19 AM
dsanders closed D45540: [globalisel][legalizerinfo] Introduce dedicated extending loads and add lowerings for them.
Apr 28 2018, 11:19 AM

Apr 27 2018

dsanders committed rL331081: Attempt to fix remaining build failures after r331071 by changing the tuple to….
Attempt to fix remaining build failures after r331071 by changing the tuple to…
Apr 27 2018, 2:06 PM
dsanders committed rL331074: Attempt to fix build failure after r331071 using std::make_tuple.
Attempt to fix build failure after r331071 using std::make_tuple
Apr 27 2018, 1:21 PM
dsanders committed rL331071: [globalisel][legalizerinfo] Add support for legalization based on the….
[globalisel][legalizerinfo] Add support for legalization based on the…
Apr 27 2018, 12:54 PM
dsanders closed D45466: [globalisel][legalizerinfo] Add support for legalization based on the MachineMemOperand.
Apr 27 2018, 12:54 PM

Apr 23 2018

Herald added a reviewer for D45543: [globalisel] Add a combiner helpers for extending loads and use them in a pre-legalize combiner for AArch64: javed.absar.

Maybe we could have all the tests in one file called prelegalize-combine-extloads.mir

Apr 23 2018, 12:46 AM

Apr 22 2018

Herald added a reviewer for D45466: [globalisel][legalizerinfo] Add support for legalization based on the MachineMemOperand: javed.absar.

Thanks. I'll commit this when I'm back in the office on Thursday

Apr 22 2018, 10:44 PM
Herald added a reviewer for D45540: [globalisel][legalizerinfo] Introduce dedicated extending loads and add lowerings for them: javed.absar.

Thanks. I'll commit this (with the nits fixed) when I'm back in the office on Thursday

Apr 22 2018, 10:44 PM
dsanders added inline comments to D36569: [globalisel][tablegen] Add support for fpimm and import of APInt/APFloat based ImmLeaf..
Apr 22 2018, 10:36 PM

Apr 18 2018

dsanders added a comment to D43962: [GlobalISel][utils] Adding the init version of Instruction Select Testgen.

Aside from being a large patch with few function-level comments, some of the non-functional changes also make this difficult to review. It would be helpful to move things like the indentation correction on testImm*(), the introduction of buildTable and getMatchTable(), the changes to coverage, moving the emission of selectImpl() down, etc. into a separate patch(es).

Apr 18 2018, 3:27 PM

Apr 17 2018

dsanders created D45734: [reassociate] Fix excessive revisits when processing long chains of reassociatable instructions..
Apr 17 2018, 1:20 PM

Apr 16 2018

dsanders accepted D45398: Fix lock order inversion between ManagedStatic and Statistic.

Sorry for the delay, I was out of the office for a few days.

Apr 16 2018, 9:22 AM

Apr 11 2018

dsanders added a dependent revision for D45541: [globalisel] Update GlobalISel emitter to match new representation of extending loads: D45543: [globalisel] Add a combiner helpers for extending loads and use them in a pre-legalize combiner for AArch64.
Apr 11 2018, 4:13 PM
dsanders created D45543: [globalisel] Add a combiner helpers for extending loads and use them in a pre-legalize combiner for AArch64.
Apr 11 2018, 4:13 PM
dsanders added a dependent revision for D45540: [globalisel][legalizerinfo] Introduce dedicated extending loads and add lowerings for them: D45541: [globalisel] Update GlobalISel emitter to match new representation of extending loads.
Apr 11 2018, 4:08 PM
dsanders created D45541: [globalisel] Update GlobalISel emitter to match new representation of extending loads.
Apr 11 2018, 4:08 PM
dsanders created D45540: [globalisel][legalizerinfo] Introduce dedicated extending loads and add lowerings for them.
Apr 11 2018, 3:47 PM
dsanders updated the diff for D45466: [globalisel][legalizerinfo] Add support for legalization based on the MachineMemOperand.

Rebase to trunk

Apr 11 2018, 3:28 PM
dsanders added inline comments to D44304: [MIPS GlobalISel] Select add i32, i32.
Apr 11 2018, 8:18 AM

Apr 9 2018

dsanders created D45466: [globalisel][legalizerinfo] Add support for legalization based on the MachineMemOperand.
Apr 9 2018, 3:46 PM
dsanders committed rL329623: [globalisel][legalizerinfo] Add support for the Lower action in….
[globalisel][legalizerinfo] Add support for the Lower action in…
Apr 9 2018, 2:13 PM
dsanders committed rL329602: Fix type mismatch between MachineMemOperand constructor and accessors. NFC.
Fix type mismatch between MachineMemOperand constructor and accessors. NFC
Apr 9 2018, 11:46 AM

Apr 6 2018

dsanders added inline comments to D45398: Fix lock order inversion between ManagedStatic and Statistic.
Apr 6 2018, 6:47 PM
dsanders accepted D45353: AMDGPU: Initialize GlobalISel passes.

I'm not sure why it's still separate but having it separate means that compilers with only targets that don't implement GlobalISel don't spend time initializing the passes for it. I think Tom's guess about minimizing LLVM_BUILD_GLOBAL_ISEL is probably the original reason.

Apr 6 2018, 9:30 AM

Apr 3 2018

dsanders added a comment to D44304: [MIPS GlobalISel] Select add i32, i32.

I think you're ok to commit

Apr 3 2018, 7:53 AM

Apr 2 2018

dsanders accepted D45144: [TableGen] Change std::sort to llvm::sort in response to r327219.

LGTM

Apr 2 2018, 8:00 AM

Mar 27 2018

dsanders added a comment to D43976: [GlobalISel][AArch64] Adding Testgen'd tests for InstructionSelect pass.

Ah, I'm reading the patches out of order :-). 1.5s seems reasonable

Mar 27 2018, 3:26 PM
dsanders added a comment to D43976: [GlobalISel][AArch64] Adding Testgen'd tests for InstructionSelect pass.

Also, could you double check the memory usage for this. Some of the bots have a fairly limited amount of memory so the 'ninja check' time will be amplified on those if it consumes a lot.

Mar 27 2018, 3:25 PM
dsanders added a comment to D43979: [GlobalISel][ARM] Adding Testgen'd tests for InstructionSelect pass.

Those are quite large tests. How much time does it add to 'ninja check'? If it's reasonably short (I don't really have a number in mind for this) then LGTM. Otherwise we may need to think about trimming it down a bit

Mar 27 2018, 3:22 PM
dsanders added a comment to D43976: [GlobalISel][AArch64] Adding Testgen'd tests for InstructionSelect pass.

Those are quite large tests. How much time does it add to 'ninja check'? If it's reasonably short (I don't really have a number in mind for this) then LGTM. Otherwise we may need to think about trimming it down a bit

Mar 27 2018, 3:22 PM
dsanders added a comment to D44704: [GlobalISel][X86][ARM] Relaxing type constraints on G_SHL and friends.

The GenericOpcodes.td change makes sense to me but I'm surprised that only AMGGPU's LegalizerInfo is changing and only for SHL. I'd have expected most (probably all) GlobalISel targets to need to change all three shifts.

Mar 27 2018, 3:16 PM

Mar 19 2018

dsanders added reviewers for D44304: [MIPS GlobalISel] Select add i32, i32: aditya_nandakumar, qcolombet.

Sorry for taking a while to get to this. It's been a busy couple weeks

Mar 19 2018, 10:55 AM

Mar 8 2018

dsanders committed rL327015: Fix unused function warning in StatisticTest.cpp.
Fix unused function warning in StatisticTest.cpp
Mar 8 2018, 7:55 AM

Mar 7 2018

dsanders committed rL326981: Support resetting STATISTIC() values using llvm::ResetStatistics().
Support resetting STATISTIC() values using llvm::ResetStatistics()
Mar 7 2018, 6:39 PM
dsanders closed D44181: Support resetting STATISTIC() values using llvm::ResetStatistics().
Mar 7 2018, 6:38 PM
dsanders updated the diff for D44181: Support resetting STATISTIC() values using llvm::ResetStatistics().

Remove unnecessary std::string constructor

Mar 7 2018, 12:09 PM
dsanders added inline comments to D44181: Support resetting STATISTIC() values using llvm::ResetStatistics().
Mar 7 2018, 11:58 AM
dsanders committed rL326936: Fix cmake's multi-config generators after r326738.
Fix cmake's multi-config generators after r326738
Mar 7 2018, 11:35 AM

Mar 6 2018

dsanders created D44181: Support resetting STATISTIC() values using llvm::ResetStatistics().
Mar 6 2018, 4:56 PM
dsanders committed rL326834: PrintStatistics() and PrintStatisticsJSON() should take StatLock.
PrintStatistics() and PrintStatisticsJSON() should take StatLock
Mar 6 2018, 1:20 PM

Mar 5 2018

dsanders committed rL326738: Re-commit: Make STATISTIC() values available programmatically.
Re-commit: Make STATISTIC() values available programmatically
Mar 5 2018, 11:40 AM
dsanders committed rL326726: Revert r326723: Make STATISTIC() values available programmatically.
Revert r326723: Make STATISTIC() values available programmatically
Mar 5 2018, 9:56 AM
dsanders committed rL326723: Make STATISTIC() values available programmatically.
Make STATISTIC() values available programmatically
Mar 5 2018, 9:44 AM
dsanders closed D43901: Make STATISTIC() values available programmatically.
Mar 5 2018, 9:44 AM

Mar 2 2018

dsanders updated the diff for D43901: Make STATISTIC() values available programmatically.

Statistics can actually be gathered (LLVM_ENABLE_STATS isn't always false anymore). Tested out of the patch using #error in the unit test.
Moved StatisticInfo back

Mar 2 2018, 2:29 PM
dsanders added inline comments to D43901: Make STATISTIC() values available programmatically.
Mar 2 2018, 2:27 PM
dsanders updated the diff for D43901: Make STATISTIC() values available programmatically.

Fix a race between GetStatistics() and addStatistic()
Documented caveats that are the callers responsibility to handle if it matters to them.

Mar 2 2018, 10:37 AM
dsanders added a comment to D43901: Make STATISTIC() values available programmatically.

LGTM

My only concern here is that I don't quite understand why only the ManagedStatic<StatisticInfo> StatInfo's writer is guarded by a mutex, but non of the readers. As the newly added GetStatistics() method leaks a read-access to StatInfo into the wild losing any control over it, it might be a problem.

Could you please clarify that a little bit?

Mar 2 2018, 9:48 AM

Mar 1 2018

dsanders updated the diff for D43901: Make STATISTIC() values available programmatically.

Remove unintentional change. ResetStatistics() is something I'm considering implementing later

Mar 1 2018, 10:52 AM