Page MenuHomePhabricator

dsanders (Daniel Sanders)
User

Projects

User does not belong to any projects.

User Details

User Since
Aug 19 2013, 3:30 PM (283 w, 2 d)

Recent Activity

Tue, Jan 22

dsanders added a comment to D55914: [GlobalISel][InstructionSelect] Add support for selecting pointer types with TableGen.

You can actually use iPTR types in patterns already, but the GlobalISel backend converts it to a scalar integer when generating the ISel code.

Tue, Jan 22, 10:40 AM
dsanders accepted D55814: GlobalISel: Support narrowing zextload/sextload.

LGTM

Tue, Jan 22, 9:42 AM

Fri, Jan 18

dsanders committed rL351572: [adt] Twine(nullptr) derefs the nullptr. Add a deleted Twine(std::nullptr_t).
[adt] Twine(nullptr) derefs the nullptr. Add a deleted Twine(std::nullptr_t)
Fri, Jan 18, 10:44 AM
dsanders closed D56870: [adt] Twine(nullptr) derefs the nullptr. Add a deleted Twine(std::nullptr_t).
Fri, Jan 18, 10:44 AM
dsanders added inline comments to D55814: GlobalISel: Support narrowing zextload/sextload.
Fri, Jan 18, 10:42 AM
dsanders accepted D55809: GlobalISel: Verify g_zextload and g_sextload.

LGTM

Fri, Jan 18, 10:31 AM

Thu, Jan 17

dsanders created D56870: [adt] Twine(nullptr) derefs the nullptr. Add a deleted Twine(std::nullptr_t).
Thu, Jan 17, 11:35 AM

Mon, Jan 7

dsanders accepted D55946: [MachineVerifier] Include offending register in allocatable live-in error msg..

LGTM

Mon, Jan 7, 2:13 PM

Fri, Jan 4

dsanders added a comment to D55842: Let TableGen write output only if it changed, instead of doing so in cmake, attempt 2.

I'm not sure this is a significant problem at the moment (I'll find out the next time I'm debugging :-)), but just to mention it. One unfortunate consequence of this is presumably that no output is written to a file when tblgen crashes because the output was only written to memory and that was lost in the crash. FWIW, being able to see the successfully-produced output has been fairly handy for me in the past as it helps to establish the context which I can then use to minimize the input or determine a strategy for debugging it. This can be particularly helpful when lldb fails to print important variables (which seems to happen often for me), and the same stack trace occurs frequently during execution. We might want to consider an option that makes it write directly to a file if that does become a problem.

Fri, Jan 4, 11:17 AM

Wed, Jan 2

dsanders committed rL350277: [tblgen][disasm] Emit record names again when decoder conflicts occur..
[tblgen][disasm] Emit record names again when decoder conflicts occur.
Wed, Jan 2, 4:18 PM

Dec 14 2018

dsanders accepted D53728: [GlobalISel] LegalizerHelper: Implement fewerElementsVector for G_LOAD/G_STORE.

LGTM

Dec 14 2018, 2:12 PM
dsanders committed rL349174: [globalisel][combiner] Fix r349167 for release mode bots.
[globalisel][combiner] Fix r349167 for release mode bots
Dec 14 2018, 10:29 AM
dsanders committed rL349167: [globalisel][combiner] Make the CombinerChangeObserver a MachineFunction….
[globalisel][combiner] Make the CombinerChangeObserver a MachineFunction…
Dec 14 2018, 9:53 AM
dsanders closed D52947: [globalisel][combiner] Make the CombinerChangeObserver a MachineFunction::Delegate.
Dec 14 2018, 9:53 AM

Dec 13 2018

dsanders added inline comments to D52947: [globalisel][combiner] Make the CombinerChangeObserver a MachineFunction::Delegate.
Dec 13 2018, 1:05 PM
dsanders committed rL349055: Recommit r349041: [tblgen][disasm] Separate encodings from instructions.
Recommit r349041: [tblgen][disasm] Separate encodings from instructions
Dec 13 2018, 8:21 AM
dsanders committed rL349046: Revert r349041: [tblgen][disasm] Separate encodings from instructions.
Revert r349041: [tblgen][disasm] Separate encodings from instructions
Dec 13 2018, 7:17 AM
dsanders committed rL349043: [mir] Fix uninitialized variable in r349035 noticed by clang-atom-d525-fedora….
[mir] Fix uninitialized variable in r349035 noticed by clang-atom-d525-fedora…
Dec 13 2018, 7:08 AM
dsanders committed rL349041: [tblgen][disasm] Separate encodings from instructions.
[tblgen][disasm] Separate encodings from instructions
Dec 13 2018, 6:59 AM
dsanders closed D52366: [tblgen][disasm] Separate encodings from instructions.
Dec 13 2018, 6:59 AM
dsanders committed rL349035: [mir] Serialize DILocation inline when not possible to use a metadata reference.
[mir] Serialize DILocation inline when not possible to use a metadata reference
Dec 13 2018, 6:28 AM
dsanders closed D55243: [mir] Serialize DILocation inline when not possible to use a metadata reference.
Dec 13 2018, 6:28 AM · debug-info
dsanders updated the diff for D52947: [globalisel][combiner] Make the CombinerChangeObserver a MachineFunction::Delegate.

Rebase and fix the remaining issue about documenting the right-to-modify
MachineInstrs within the specified function and making the non-const ptr
scheduling have consistent requirements with the const ptr scheduling.

Dec 13 2018, 5:59 AM
dsanders accepted D53728: [GlobalISel] LegalizerHelper: Implement fewerElementsVector for G_LOAD/G_STORE.

LGTM. The only comment I have is an observation that odd-length vectors generally don't fit well into fewerElements at the moment but that's not for this patch

Dec 13 2018, 4:20 AM

Dec 12 2018

dsanders committed rL348992: [globalisel] Add GISelChangeObserver::changingInstr().
[globalisel] Add GISelChangeObserver::changingInstr()
Dec 12 2018, 3:51 PM
dsanders closed D55623: [globalisel] Add GISelChangeObserver::changingInstr().
Dec 12 2018, 3:51 PM
dsanders created D55623: [globalisel] Add GISelChangeObserver::changingInstr().
Dec 12 2018, 2:33 PM
dsanders committed rL348976: [globalisel] Rename GISelChangeObserver's erasedInstr() to erasingInstr() and….
[globalisel] Rename GISelChangeObserver's erasedInstr() to erasingInstr() and…
Dec 12 2018, 1:35 PM
dsanders closed D55611: [globalisel] Rename GISelChangeObserver's erasedInstr() to erasingInstr() and related nits. NFC.
Dec 12 2018, 1:35 PM
dsanders retitled D55611: [globalisel] Rename GISelChangeObserver's erasedInstr() to erasingInstr() and related nits. NFC from Rename GISelChangeObserver's erasedInstr() to erasingInstr() and related nits. NFC to [globalisel] Rename GISelChangeObserver's erasedInstr() to erasingInstr() and related nits. NFC.
Dec 12 2018, 1:35 PM
dsanders created D55611: [globalisel] Rename GISelChangeObserver's erasedInstr() to erasingInstr() and related nits. NFC.
Dec 12 2018, 12:47 PM

Dec 7 2018

dsanders added a reviewer for D52954: Annotate timeline in Instruments with passes and other timed regions.: bogner.
Dec 7 2018, 10:33 AM

Dec 3 2018

dsanders added inline comments to D55243: [mir] Serialize DILocation inline when not possible to use a metadata reference.
Dec 3 2018, 6:17 PM · debug-info
dsanders created D55243: [mir] Serialize DILocation inline when not possible to use a metadata reference.
Dec 3 2018, 5:20 PM · debug-info

Nov 28 2018

dsanders added inline comments to D52947: [globalisel][combiner] Make the CombinerChangeObserver a MachineFunction::Delegate.
Nov 28 2018, 3:04 PM
dsanders updated the diff for D52947: [globalisel][combiner] Make the CombinerChangeObserver a MachineFunction::Delegate.

Obey constraints in replaceRegWith()
Correct the extending load combine use of replaceRegWith where we replace the
input of an extend to a wider-than-preferred type. Previously we were calling
replaceRegWith() multiple times for the same reg and changing the types of the
vreg in the process. We'd always fix it by the time we were finished but we have
asserts checking the intermediates now.

Nov 28 2018, 2:56 PM

Nov 27 2018

dsanders added a comment to D54580: [MIPS GlobalISel] Lower G_UADDE and narrowScalar G_ADD.

Hi, what happens here (when we use -run-pass) is that mir input given in test is not same as MachineFunction given to legalizer. The reason looks to be that constructors for objects are not called in same order.

This is part of MIR input.

name:            add_i128
alignment:       2
tracksRegLiveness: true
fixedStack:
  - { id: 0, offset: 28, size: 4, alignment: 4, stack-id: 0, isImmutable: true }
  - { id: 1, offset: 24, size: 4, alignment: 8, stack-id: 0, isImmutable: true }
  - { id: 2, offset: 20, size: 4, alignment: 4, stack-id: 0, isImmutable: true }
  - { id: 3, offset: 16, size: 4, alignment: 8, stack-id: 0, isImmutable: true }
body:             |
  bb.1.entry:
    liveins: $a0, $a1, $a2, $a3

    %2:_(s32) = COPY $a0
    ...
    %0:_(s128) = G_MERGE_VALUES %2(s32), %3(s32), %4(s32), %5(s32)
    %10:_(p0) = G_FRAME_INDEX %fixed-stack.3
    %6:_(s32) = G_LOAD %10(p0) :: (load 4 from %fixed-stack.3, align 0)
    ...
    %1:_(s128) = G_MERGE_VALUES %6(s32), %7(s32), %8(s32), %9(s32)
    %14:_(s128) = G_ADD %1, %0
    ...

It looks that we parse line by line and call createGenericVirtualRegister (defs in input go %2, %3, %4, %5, %0, while in reconstructed MF they go %0, %1, %2,...).

Nov 27 2018, 3:10 PM
dsanders added a comment to D54580: [MIPS GlobalISel] Lower G_UADDE and narrowScalar G_ADD.

Sorry for the belated reply.

In general, the patch is looking good to me. Unfortunately I cannot comment the note about fixedStack because do not know this part of code good enough. Probably you need to extend the list of reviewers or ask a separate question on the llvm-dev mail list.

Nov 27 2018, 8:32 AM

Nov 8 2018

dsanders created D54286: [globalisel] Early, incomplete prototype of tablegen-erated combine rules.
Nov 8 2018, 5:34 PM
dsanders added a comment to D54174: [GlobalISel] LegalizationArtifactCombiner: Combine aext([asz]ext x) -> [asz]ext x.

What's the reason for not putting this in a post-legalize combiner? It seems to me that we'll have to implement this in the Legalizer and the Combiner

Nov 8 2018, 8:40 AM
dsanders accepted D54255: [MIPS GlobalISel] narrowScalar G_CONSTANT.

LGTM

Nov 8 2018, 8:33 AM

Nov 7 2018

dsanders committed rL346368: Add 'REQUIRES: default_triple' to test/CodeGen/MIR/X86/zero-probability.mir.
Add 'REQUIRES: default_triple' to test/CodeGen/MIR/X86/zero-probability.mir
Nov 7 2018, 3:38 PM

Nov 6 2018

dsanders added inline comments to D53629: [GlobalISel] Restrict G_MERGE_VALUES capability and replace with new opcodes.
Nov 6 2018, 5:53 PM

Nov 2 2018

dsanders added a comment to D53594: [GlobalISel] Introduce G_BUILD_VECTOR and G_CONCAT_VECTOR opcodes.

I'm not sure how this is any better. If we had an opcode G_BUILD_VECTOR_TRUNC, it would have a superset of the functionality of the G_BUILD_VECTOR. You'd still have this powerful opcode, except now you have potential code duplication in handling the two, as well as a hindered G_BUILD_VECTOR.

Unless you're say that G_BUILD_VECTOR _TRUNC can *only* deal with oversize scalars, in which case it's an odd opcode for a corner case (and I would suggest here that we just deal with the inconvenient types, but I'm not married to the idea).

I was suggesting the later.

Generally speaking, I am not a fan of implicit stuff, thus the proposal (and that's easy to have a isBuildVectorLike or something). That said, I'd like to go back as why we need to do that.

So the problem was that we wanted to represent:
a(<4 x s32>) = G_BUILD_VECTOR p(s32), q, r, s
b(<4 x s8>) = G_TRUNC a

into
b(< 4 x s8>) = G_BUILD_VECTOR p(s32), q, r, s

Why can't we keep the "verbose" representation?

Nov 2 2018, 10:20 AM
dsanders added a comment to D53203: Allow MemoryLocation to carry pre-existing knowledge to AA to elide expensive repeated checks.

I'm not very familiar with the MemorySSA pass but based on a fairly quick skim of it ...
It looks like the MemoryDef/MemoryUse objects are attached to the BB rather than the instructions.

I'll write more later, but quickly, this is incorrect. MemorySSA has per-instruction granularity.

Nov 2 2018, 9:08 AM

Nov 1 2018

dsanders added a comment to D53203: Allow MemoryLocation to carry pre-existing knowledge to AA to elide expensive repeated checks.

Hi Hal,

I didn't spot you in CODE_OWNERS.txt at first because I was searching for 'Alias' instead of 'alias' :-). Would you be able to take a look at this patch too?

Thanks. I suppose that I should echo Chris's question about the proposed local-dominance cache with respect to this change too: If we switch this pass to using MemorySSA does this problem go away?

Nov 1 2018, 5:25 PM
dsanders added inline comments to D53734: [GlobalISel] Fix a bug in LegalizeRuleSet::clampMaxNumElements.
Nov 1 2018, 11:56 AM
dsanders accepted D53734: [GlobalISel] Fix a bug in LegalizeRuleSet::clampMaxNumElements.

LGTM with a couple testing nits

Nov 1 2018, 10:04 AM
dsanders added a comment to D53938: [MC] Implement EmitRawText in MCNullStreamer.

Could you commit this patch on my behalf?

Nov 1 2018, 8:45 AM
dsanders committed rL345841: [MC] Implement EmitRawText in MCNullStreamer.
[MC] Implement EmitRawText in MCNullStreamer
Nov 1 2018, 8:43 AM
dsanders closed D53938: [MC] Implement EmitRawText in MCNullStreamer.
Nov 1 2018, 8:43 AM
dsanders updated the summary of D53938: [MC] Implement EmitRawText in MCNullStreamer.
Nov 1 2018, 8:28 AM

Oct 31 2018

dsanders updated the diff for D53203: Allow MemoryLocation to carry pre-existing knowledge to AA to elide expensive repeated checks.

Add the sanity checks

Oct 31 2018, 5:37 PM
dsanders added a comment to D53203: Allow MemoryLocation to carry pre-existing knowledge to AA to elide expensive repeated checks.

Thanks Alina

Oct 31 2018, 5:36 PM
dsanders added inline comments to D53416: [MIPS GlobalISel] Select sub.
Oct 31 2018, 1:27 PM
dsanders committed rL345772: [adt] SparseBitVector::test() should be const.
[adt] SparseBitVector::test() should be const
Oct 31 2018, 1:08 PM
dsanders closed D53447: [adt] SparseBitVector::test() should be const.
Oct 31 2018, 1:08 PM
dsanders updated the summary of D53447: [adt] SparseBitVector::test() should be const.
Oct 31 2018, 12:54 PM
dsanders committed rL345769: [globalisel] Add comments indicating the operand order.
[globalisel] Add comments indicating the operand order
Oct 31 2018, 12:52 PM
dsanders updated the diff for D52366: [tblgen][disasm] Separate encodings from instructions.

Fixed the nits about not using the << operator but doing the same thing it does

Oct 31 2018, 11:40 AM
dsanders committed rL345754: [globalisel][irtranslator] Fix test from r345743 on non-asserts builds..
[globalisel][irtranslator] Fix test from r345743 on non-asserts builds.
Oct 31 2018, 11:01 AM
dsanders added a comment to D53447: [adt] SparseBitVector::test() should be const.

Thanks

Oct 31 2018, 10:52 AM
dsanders added inline comments to D52366: [tblgen][disasm] Separate encodings from instructions.
Oct 31 2018, 10:42 AM
dsanders committed rL345743: [globalisel][irtranslator] Verify that DILocations aren't lost in translation.
[globalisel][irtranslator] Verify that DILocations aren't lost in translation
Oct 31 2018, 10:34 AM
dsanders closed D53740: [globalisel][irtranslator] Verify that DILocations aren't lost in translation.
Oct 31 2018, 10:34 AM
dsanders accepted D53938: [MC] Implement EmitRawText in MCNullStreamer.

LGTM. It makes sense to me that MCNullStreamer supports raw text by not emitting anything.

Oct 31 2018, 10:22 AM
dsanders added a comment to D53203: Allow MemoryLocation to carry pre-existing knowledge to AA to elide expensive repeated checks.

Hi Hal,

Oct 31 2018, 10:14 AM
dsanders added a reviewer for D53203: Allow MemoryLocation to carry pre-existing knowledge to AA to elide expensive repeated checks: hfinkel.
Oct 31 2018, 10:11 AM
dsanders added a comment to D53203: Allow MemoryLocation to carry pre-existing knowledge to AA to elide expensive repeated checks.

Hi Sanjoy,

Oct 31 2018, 10:00 AM
dsanders added a reviewer for D53203: Allow MemoryLocation to carry pre-existing knowledge to AA to elide expensive repeated checks: sanjoy.
Oct 31 2018, 9:58 AM
dsanders added inline comments to D53416: [MIPS GlobalISel] Select sub.
Oct 31 2018, 9:23 AM

Oct 29 2018

dsanders added inline comments to D53740: [globalisel][irtranslator] Verify that DILocations aren't lost in translation.
Oct 29 2018, 9:46 AM
dsanders added a comment to D52100: [tblgen] Allow FixedLenDecoderEmitter to use APInt-like objects as InsnType.

I've been thinking about this over the weekend and I think I may be able to hazard a guess: Are you trying to use APInt directly? Or use APInt-like object that meets all the requirements in the emitted comment? (see emitFieldFromInstruction)

Oct 29 2018, 9:35 AM
dsanders added inline comments to D53740: [globalisel][irtranslator] Verify that DILocations aren't lost in translation.
Oct 29 2018, 8:29 AM

Oct 26 2018

dsanders added a comment to D52100: [tblgen] Allow FixedLenDecoderEmitter to use APInt-like objects as InsnType.

This commit breaks my 128bit instruction target.
I'm currently using a workaround locally for enabling APInt based encodings when your instruction encoding is >64bit.
The main issue with this approach is that getBinaryCodeForInstr completely falls apart.

Could you elaborate on this? getBinaryCodeForInstr() is for encoding instructions whereas FixedLenDecoderEmitter generates a decoder so I'm surprised that you're seeing a connection between getBinaryCodeForInstr() and this patch. Our out-of-tree target didn't use the FixedLenDecoderEmitter until after this patch made it possible but we've been using getBinaryCodeForInstr() for a very long time so I think there's unlikely to be a connection between the two. Additionally, not affecting encoding was a requirement for us as we're also working around the lack of >64-bit support in getBinaryCodeForInstr()

I can only assume you aren't using a disassembler in your backend currently? That also breaks immediately.

Oct 26 2018, 6:14 PM
dsanders added a comment to D52100: [tblgen] Allow FixedLenDecoderEmitter to use APInt-like objects as InsnType.

This commit breaks my 128bit instruction target.
I'm currently using a workaround locally for enabling APInt based encodings when your instruction encoding is >64bit.
The main issue with this approach is that getBinaryCodeForInstr completely falls apart.

Oct 26 2018, 2:36 PM

Oct 25 2018

dsanders created D53740: [globalisel][irtranslator] Verify that DILocations aren't lost in translation.
Oct 25 2018, 4:30 PM
dsanders accepted D53664: [GlobalISel] LegalizerHelper: Fix the incorrect alignment when splitting loads/stores in narrowScalar.

LGTM

Oct 25 2018, 10:41 AM
dsanders added inline comments to D53629: [GlobalISel] Restrict G_MERGE_VALUES capability and replace with new opcodes.
Oct 25 2018, 10:31 AM

Oct 24 2018

dsanders accepted D53679: [AArch64][GlobalISel] Fix the LegalityPredicate for lowerIf for G_LOAD/G_STORE.

LGTM with the test nit

Oct 24 2018, 6:45 PM

Oct 23 2018

dsanders committed rL345059: Fix MSVC build by correcting placement of declspec after r345056.
Fix MSVC build by correcting placement of declspec after r345056
Oct 23 2018, 10:43 AM
dsanders committed rL345056: [tblgen] Allow FixedLenDecoderEmitter to use APInt-like objects as InsnType.
[tblgen] Allow FixedLenDecoderEmitter to use APInt-like objects as InsnType
Oct 23 2018, 10:25 AM
dsanders closed D52100: [tblgen] Allow FixedLenDecoderEmitter to use APInt-like objects as InsnType.
Oct 23 2018, 10:25 AM
dsanders added a comment to D52100: [tblgen] Allow FixedLenDecoderEmitter to use APInt-like objects as InsnType.

Thanks

Oct 23 2018, 10:05 AM
dsanders updated the diff for D52366: [tblgen][disasm] Separate encodings from instructions.

Fold in another correction that ended up in a downstream-specific patch:
Only emit 'FOO => BAR' comments encodings caused by AdditionalEncoding. For Instruction, keep the existing 'FOO' comments.

Oct 23 2018, 10:04 AM
dsanders updated the diff for D52100: [tblgen] Allow FixedLenDecoderEmitter to use APInt-like objects as InsnType.

Fold in a correction that ended up in a downstream-specific patch. The 64-bit assertion should only be in the path for integer types

Oct 23 2018, 10:01 AM
dsanders added inline comments to rL336334: [TableGen] Increase the number of supported decoder fix-ups..
Oct 23 2018, 9:53 AM
dsanders updated the diff for D52366: [tblgen][disasm] Separate encodings from instructions.

Rebase

Oct 23 2018, 9:32 AM
dsanders updated the diff for D52100: [tblgen] Allow FixedLenDecoderEmitter to use APInt-like objects as InsnType.

Rebase. I think the incorrect assertion is a missing patch from my original patch series. The tip of my local repo only has that assertion in the integral case

Oct 23 2018, 9:26 AM

Oct 22 2018

dsanders added inline comments to D52369: [tblgen][disasm] Allow multiple encodings to disassemble to the same instruction.
Oct 22 2018, 9:34 AM
dsanders added inline comments to D52100: [tblgen] Allow FixedLenDecoderEmitter to use APInt-like objects as InsnType.
Oct 22 2018, 9:25 AM
dsanders added inline comments to D53490: [MIR] Provide a default alignment for stack objects.
Oct 22 2018, 8:59 AM

Oct 19 2018

dsanders added a comment to D52366: [tblgen][disasm] Separate encodings from instructions.

The encoding of an instruction includes encodings of its operands. How are you planning to implement separate encoder/decoder methods? Right now their names are all embedded into operand definitions.

Oct 19 2018, 3:28 PM
dsanders added a comment to D52369: [tblgen][disasm] Allow multiple encodings to disassemble to the same instruction.

ping

Oct 19 2018, 1:25 PM
dsanders added a comment to D52366: [tblgen][disasm] Separate encodings from instructions.

ping

Oct 19 2018, 1:25 PM
dsanders added a comment to D52100: [tblgen] Allow FixedLenDecoderEmitter to use APInt-like objects as InsnType.

ping

Oct 19 2018, 1:25 PM
dsanders created D53447: [adt] SparseBitVector::test() should be const.
Oct 19 2018, 12:00 PM
dsanders added inline comments to D53416: [MIPS GlobalISel] Select sub.
Oct 19 2018, 8:53 AM

Oct 16 2018

dsanders accepted D53304: [GISel]: Allow Unused G_PHIs to be DCEd.

LGTM with a nit. Also we should consider whether we ought to be calling isSafeToMove() when we're not moving things.

Oct 16 2018, 8:46 AM

Oct 15 2018

dsanders added inline comments to D52947: [globalisel][combiner] Make the CombinerChangeObserver a MachineFunction::Delegate.
Oct 15 2018, 3:03 PM