In D55914#1364207, @tstellar wrote:

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

LGTM

LGTM

LGTM

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.

LGTM

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.

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

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.

In D54580#1310009, @Petar.Avramovic wrote:

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,...).

In D54580#1309484, @atanasyan wrote:

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.

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

LGTM

In D53594#1283016, @qcolombet wrote:

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?

In D53203#1285430, @hfinkel wrote:

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.

In D53203#1283422, @hfinkel wrote:

In D53203#1282454, @dsanders wrote:

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?

LGTM with a couple testing nits

In D53938#1283840, @eush wrote:

Could you commit this patch on my behalf?

Add the sanity checks

Thanks Alina

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

Thanks

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

Hi Hal,

Hi Sanjoy,

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)

In D52100#1277915, @Sonicadvance1 wrote:

In D52100#1277817, @dsanders wrote:

In D52100#1277769, @Sonicadvance1 wrote:

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.

In D52100#1277769, @Sonicadvance1 wrote:

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.

LGTM

LGTM with the test nit

Thanks

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.

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

Rebase

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

In D52366#1269627, @kparzysz wrote:

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.

ping

ping

ping

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