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.

So with this change, if you have:

So overall I think we should have some form of IR canonicalization before translation for the majority of cases to be handled for -O0.

In D51362#1219573, @qcolombet wrote:

Going back to a higher-level description for all that stuff, I wonder if ISel could already handle this.

Indeed, it seems easy, at least conceptually, to teach tablegen to check for the commuted patterns to get the immediate version whenever possible before trying the non-immediate variant.

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

In D51362#1218451, @rtereshin wrote:

In D51362#1218394, @aemerson wrote:

I'm not against a separate IR canonicalization stage, even though it's a bit overkill for this particular case. At the moment it looks like InstCombine is doing the canonicalizaton for this case, so re-running that is out of the question. A new pass looks to be on the cards. Anyone else have opinions on this?

Does that mean that something after the last InstCombine produces icmp's with the non-canonical operand order? If so maybe that something needs to be chased down and fixed instead.

I'm not against a separate IR canonicalization stage, even though it's a bit overkill for this particular case. At the moment it looks like InstCombine is doing the canonicalizaton for this case, so re-running that is out of the question. A new pass looks to be on the cards. Anyone else have opinions on this?

In D51362#1216865, @qcolombet wrote:

Hi Amara,

That patch worries me, because I feel that we are going to pull all the LLVM IR code that does canonicalization.
The way I see it, is the input IR should already be in a canonical form and thus we don't have to do that.

If that's not the case, I would argue that bad output code is fine (garbage in, garbage out).

What do you think?

Cheers,
-Quentin

I'd like to get this in. LGTM but needs one issue addressed.

In D51197#1215516, @igorb wrote:

In D51197#1215499, @aemerson wrote:

x86 is still using UADDE, and it's generated only by IRTranslator, so if you do this then x86 will lose support for compiling llvm.uadd.with.overflow intrinsics. @igorb what do you think?

Which is exposing a hole in our general GISel testing, where individual changes in passes can result in overall support for some input IR to be lost.

Hi,
I think currently X86 use G_UADDE generated by Legalizer to support 64bit add on 32bit architecture.

Regards,
Igor

x86 is still using UADDE, and it's generated only by IRTranslator, so if you do this then x86 will lose support for compiling llvm.uadd.with.overflow intrinsics. @igorb what do you think?

Looks like Lang added the original code, but LGTM anyway. I even think we did this exact change downstream a few years ago at ARM...

Sure, done.

I'll take over and fix this in the way I suggested. Thanks for reporting this!

I'm a little uneasy about specifying libm in the names of the intrinsics. The langref mentions libm in the description, but the semantics don't exactly match due to ignoring errno. That said, I don't really have a suggestion for a better name, so the only thing I can ask is that we make it a bit more explicit in the documentation what we mean by "LIBM" here.

Thinking about this more, I can't think of any reason why we'd ever want to add offsets to an existing offset list. It would probably be better if valueIsSpit cleared the vector (and the doc comment made this clear).

Could you upload with more context please, and a test case?

Thanks for taking this on. I think some x86 tests would also be good here as there evidently isn't much coverage at the moment.

Hi Daniel, sorry for the delay.