This should be abandoned?

LGTM, thanks.

In D56738#1358417, @fhahn wrote:

In D56738#1358379, @lebedev.ri wrote:

Can any test case be added here, or would it be just too big to realistically test?

I have a file triggering the assert, but I think the cases where it is triggered require 64k load/stores in a single BB. But maybe there is another way to construct a node with more than 64k operands?

Just checking, this is dependent on D55639?

In D52803#1349373, @kristof.beyls wrote:

In D52803#1349005, @aditya_nandakumar wrote:

By default the CSE is enabled during IRTranslator and Legalizer now (disable with -enable-cse-in-legalizer/irtranslator=0).

I naively wonder if unconditionally enabling a CSE optimization even at -O0 is going to lead to a poorer debug experience in some cases?

This is a good point, so I think that if we enable it for -O0 it should only be for constants.

I've also updated most tests (except some tests where it's not obvious what the test is doing where CSE is disabled).

If CSE should be enabled by default, I guess that we'll need to figure out what the tests are supposed to be doing so that there aren't many tests mainly testing non-default behaviour?

LGTM.

In D55639#1352510, @arsenm wrote:

In D55639#1350196, @aemerson wrote:

In D55639#1349093, @arsenm wrote:

In D55639#1348912, @aemerson wrote:

Thanks, this definitely needed to be done. Looks mostly fine, but I also can't be confident on the X86 changes. @igorb @aivchenk can you take a look at this change?

Do you have an opinion how how to handle selecting the shift amount type in the combiner?

For AArch64 we want shift immediates to be 64 bit in order to select using the imported patterns. I can see 3 options:
a) we ask the target via a hook what the preferred shift immediate type is
b) we iteratively check from i64->i32(maybe -> i16?) using the isInstUnsupported() call to see which type combination is legal and pick the first
c) we don't worry about the type and get the legalizer to re-legalize it. I don't think however that the legalizer artifact combiner currently tries to re-legalize the combined instructions. @aditya_nandakumar do you have an opinion?

For now, my slight preference would be option b) as its simpler and doesn't try to change the way the legalizer/combiner currently works.

This does assume a power of 2 legality

In D55639#1349093, @arsenm wrote:

In D55639#1348912, @aemerson wrote:

Thanks, this definitely needed to be done. Looks mostly fine, but I also can't be confident on the X86 changes. @igorb @aivchenk can you take a look at this change?

Do you have an opinion how how to handle selecting the shift amount type in the combiner?

Thanks, this definitely needed to be done. Looks mostly fine, but I also can't be confident on the X86 changes. @igorb @aivchenk can you take a look at this change?

LGTM.

This is strange. G_CONSTANT isn't supposed to be able to splat the constant into a vector, so this is an oversight in the MachineVerifier for not catching this case.

Thanks for the cleanup. LGTM with minor naming change.

LGTM with minor change.

LGTM. In general for IR translator tests we don't need to check for every type combination if it's a simple 1-1 mapping, but no need to change the patch.

I'd like to revive this patch, as the other IR canonicalization pass is much more controversial.

Thanks, committed in r348788.

The patch title should probably say adding IR translation support instead of selection support. If we're adding selection support too then we should have a test case, otherwise it can be in a later patch.

LGTM.

In D54710#1319529, @spatel wrote:

In D54710#1319436, @aemerson wrote:

This seems to me to be a bug in ProcessImplicitDefs, for some reasoning killing the IMPLICIT_DEF even though there's a subreg use. GlobalISel MIR output looks fine to me.

Regardless of whether there's a bug in ProcessImplicitDefs, we would have to correctly propagate the undef flag through this transform (from 'add' to 'lea') though? I'm proposing to avoid that for this case based on existing code that appears to do the same thing.

This seems to me to be a bug in ProcessImplicitDefs, for some reasoning killing the IMPLICIT_DEF even though there's a subreg use. GlobalISel MIR output looks fine to me.

Updated to incorporate Volkan's patches to change buildMerge calls to explicitly check for types before choosing the right build method. Also added test for and fixed artifact combiner for G_BUILD_VECTOR.

Addressed comments, deleted the non power of 2 test.

Changed the approach to replace values in PendingLoads in place. Also added an assert in SelectionDAG::createOperands() to check if we can store the requested number of operands.

In D55073#1313703, @RKSimon wrote:

Does this fix PR7250 and PR37000 ? Those test cases look usable.

Hi Petr,

So these extends are created by the legalizer, correct? If so, it seems ok to me as that's what these artifact combines were intended for.

Added comment for G_BUILD_VECTOR_TRUNC and tests for the verifier. I haven't added all the different combinations because we can only write one test per file, so I tested the most useful of the verifier errors.

Updated patch to introduce a separate G_BUILD_VECTOR_TRUNC opcode.

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?
I would expect it would be easy to handle that in ISel.

As long as <4 x s32> = the G_BUILD_VECTOR could be legalized I think the worst that will happen is that we just get poor codegen.

Also, just throwing that here, I haven't really thought about it: should we rethink our legality model to check patterns instead of one instruction at a time?
I.e., maybe

a(s8) = ...

is not legal but

a(s8) = ...
b(<4 x s8>) = build_vector a(s8)

is.

I think the issue of sequences becoming illegal because of things like copies being introduced is still a deal breaker for that, and it would also introduce a code motion barrier since we can't select sequences spanning blocks. E.g. it wouldn't be safe to move the insn in the predecessor due to other uses.

This actually ties back to one question that I don't think we answered. Do we legalize on the type (i.e., s8 to s32) or the size and type can be different (i.e., s8(8b) to s8(32b))?
It seems to me we are trying to write <4 x s8> = s8(32b), ...., but maybe we want <4 x s8> = s32, ....

I've had a think about what having separate type and container sizes would actually mean. For brevity, let’s call values which have distinct type and container sizes “partial values”. A partial value s8:s32 means that the upper 24 bits of this 32 bit register is always undef. For some operations, it should always be possible to ignore the type size and just use the container size for selection. E.g. non-flag setting G_ADD could select a 32 bit add and satisfy the semantics of an s8 add. But for other operations like shifts, or count-leading-zeros, we can't use a wider operation type, so the entire chain of partial value operations would be restricted to selecting the narrower type, rather than the container. And at that point, the container type may as well not be there. There may be a way around this issue I haven't seen though.

Anyhow, to be concrete, I am fine with the implicit truncate, we know this is not too bad based on SDISel experience, I want to make sure we consider all our options here.

Sure. Given that we already have this implicit truncating behavior on account of G_MERGE being an omnipotent operation, should we allow the implicit truncation with G_BUILD_VECTOR for now? If we have problems with it later we can revisit the idea of adding a separate opcode/something else.

Cheers,
-Quentin

In D53594#1280811, @qcolombet wrote:

Thanks @aditya_nandakumar for the context.

I understand where you're coming from.

What I meant is that I would rather have one opcode of build_vector and one for build_vector with trunc. The rationale is that it makes it clear what you want to achieve and it makes for a stronger type verifier.

LGTM with the nits addressed.

Updated AArch64 legalization. I've removed the unsupported case since we're actually making it valid.

Updated patch to allow G_BUILD_VECTOR to have scalar operands not identical to the result vector element types. This allows implicit truncation.

Committed in r345265.

+ @huntergr On a tangential note, G_BUILD_VECTOR for expressing broadcasts of scalars won't work for SVE and other variable vector width targets. We do however still leave the door open here for a new G_BROADCAST opcode in future.

In D53594#1273910, @nhaehnle wrote:

Are two different opcodes for this really needed? Why not a single opcode with uniformly typed operands, either scalar or vector, which concatenates the operands as an appropriately sized vector? I know there's precedent for the separation elsewhere in LLVM, but it has always seemed redundant to me personally. The code for handling the BUILD vs. CONCAT cases tends to be very similar.

Thanks for working on this. Apart from Daniel's suggestion, I have a few more comments but haven't done an in depth analysis of the memory costs/algorithmic properties.

Ping.

@aditya_nandakumar I've added a target hook that defaults to enabling this.

This seems like something that most targets would want done by default. I wonder if we should have a standard set of legalizer actions that targets can use. LGTM anyway.

In D51953#1237061, @aditya_nandakumar wrote:

Hi Amara - would it be possible to add this as part of AArch64 pass pipeline? Adding this to all targets would result in needless burning of compile time for targets that don't need this.

This isn't really ideal because of how trivial it is currently. The issue is that if we added the capability to InstSimplify (which is where it would fit most naturally), we'd still end up with the problem of running the whole of InstSimplify in the GlobalISel pipeline. It's reasonable for now I think so I'll commit with the changes requested.

Looks ok, if a bit cumbersome with the additional state (do we need to hold references to it?). I'd like to see how this state is used with the CSE builder first though.

Ok, I'm surprised that would be a realistic scenario but I'll make the change. Thanks.

You're right, that's a simplified expression.

Ping.

Hi Quentin,

In D51145#1224873, @spatel wrote:

In D51145#1223770, @mcberg2017 wrote:

I added one more non fmf instruction and there is room to add others if needed.

There's also discussion about the definition of FPMathOperator and the relation to FMF here:
https://bugs.llvm.org/show_bug.cgi?id=38086
D51646

Ok, to me it seems like instructions for which fast math flags can't apply shouldn't be classed as FPMathOperators. insertelement/extractelement are just moving raw bits. If we disallowed those instruction types in the FPMathOperator classof() then this problem should go away, and we don't incorrectly relax FP semantics like in the case I pointed out.

I’m not seeing how FP instructions can not carry flags. The IR semantics are defined to be strict FP unless explicitly relaxed.

In D51362#1221028, @qcolombet wrote:

It can't do that all the time, with these G_ICMPs for example it also has to swap the predicate (not that we even have imported patterns that could possibly match for AArch64).

Why not?
I understand G_ICMPs are special, but we could come up with whatever complicated logic in TableGen.

Hit submit too early:
So with the above code B will not have the flag cleared? If that's the case it doesn't seem right to me.