@qcolombet @nickdesaulniers Have addressed all the queries

Currently, depending upon whether the add/sub instruction can synthesize immediate directly, its decided whether to generate add/sub+immediate or mov+mla/mad/msb/mls ops.

@paulwalker-arm Any suggestions/improvements ?

This is reland attempt.

In D145657#4194133, @nickdesaulniers wrote:

Thanks for taking over D42600!

Please drop

D42600 couldnt land for some reasons.

from the commit message. It's irrelevant now that you've commandeered D42600.

Consider making this test into a MIR test that uses -run-only=shrink-wrap rather than testing the entire backend. You can use llc -stop-before=<passname> to dump MIR, and llc -start-before=<passname> to restart the whole backend from a given starting pass (or more simply -run-only=<passname> to test one backend pass in isolation. I strongly encourage you to change this test, but wont block landing this or D42600 on it.

Approving, but please do not commit until D42600 has been approved, and please commit them together at roughly the same time (keep them two distinct commits though).

@paulwalker-arm

for e = 0 to elements-1
    if ElemP[mask, e, esize] == '1' then
        integer element1 = UInt(Elem[operand1, e, esize]);
        integer element2 = UInt(Elem[operand2, e, esize]);
        integer product = element1 * element2;
        if sub_op then
            Elem[result, e, esize] = Elem[operand3, e, esize] - product;
        else
            Elem[result, e, esize] = Elem[operand3, e, esize] + product;
    else
        Elem[result, e, esize] = Elem[**operand1**, e, esize];

In D145472#4174810, @fhahn wrote:

LGTM, thanks! The commit title could be a bit more descriptive by explicitly mentioning what the code change is, e.g. something like: [LV] Use SetVector to collect invariants in calculateRegisterUsage.

Also, this is not AArch64 specific, so it would be good to drop the [AArch64] part.

@paulkirth Trying to address the issue you have mentioned in D145472. Thanks

In D142998#4170949, @paulwalker-arm wrote:

In D142998#4170452, @sushgokh wrote:

@paulwalker-arm Any comments/suggestions on this ?

In truth I've ignored this patch whilst waiting on your other patch. My review comments on D142656 include the test file, which I'm recommending to follow the same structure as was used for the FMAs. Those being simple tests I'm happy for them to remain included with the patch that improves the isel. I think this patch can then be rebased on top of D142656 to clearly show the extra cases you care about.

@paulwalker-arm Any comments/suggestions on this ?

@junbuml I will take over this work next week.

@sdesmalen I couldnt simplify the crashing test further. I hope this suffices

@paulwalker-arm adding test cases for

1. Generating pseudo inst for MLA/MAD/MLS/MSUB
2. Basic test cases on mul + add/sub as you had suggested in other patch

@sdesmalen Have updated the patch. Thanks for the explanation

any suggestions/comments on the patch ?

@paulwalker-arm can you check now if this looks good ?

Hi,
Could someone please have a look at the patch?

In D142656#4126789, @paulwalker-arm wrote:

The patch looks much better now, thanks @sushgokh. I'll take a proper look tomorrow but can I draw your attention to D143764 which I plan to land tomorrow. It shows a style that equally applies to your patch as well to reduce the number of patterns within AArch64SVEInstrInfo.td.

Changed the logic to generate Psuedo instruction at ISel(which would be changed to actual instruction much later during ExpandPseudo pass) upon @paulwalker-arm suggestion

Adding a loop based test case to understand the effect of patch D142656 better

@junbuml Is it possible for you to rebase this patch and upstream it? With current LLVM trunk, its giving around 7% uplift on SPEC2017 povray

One thing I missed out is a check if the invariant is taking part in vector ops. If its indeed taking part in vector ops, we will need vector registers for it. But, if it doesnt, then scalar registers should be sufficient.

In D143422#4108881, @madhur13490 wrote:

I see the problem of calculating incorrect register usage. However, is it just limited to loop invariants?

@paulwalker-arm Thanks for the explanation. Let me check the example you have given and come up with implementation. If I cannot, will ask for your help

1. Uploading the full context
2. Added a check that one of the addends is constant splat
3. Made some cleanup
4. Updated test for precommit test in D142998

In D142656#4096107, @SjoerdMeijer wrote:

And for completeness, summarising previous comments, these are my other 2 concerns.

We need an alternative for:

for(SDNode* use: Op1->uses())

This is not doing what we want. So my question here is why we need it, why a hasOneUse check won't suffice? What is the motivation test case? If we know that, perhaps we can have a think about this.

Second, this could be related to the above, but I remember from some tests that we are missing a few opportunities. Why is that? What would be needed to recognises these cases? Reason I am asking is to see if this is the right place to do this thing.

In D142656#4096082, @david-arm wrote:

Hi @SjoerdMeijer, in @sushgokh's defence there is precedent for some of the changes in this patch - by changing from SVE_4_Op_Pat to SVE_4_Mad_Op_Pat we are able to set the AddedComplexity field to the pattern, which is not dissimilar to SVE_3_Op_Pat_SelZero or SVE_3_Op_Pat_Shift_Imm_SelZero, i.e.

let AddedComplexity = 1 in {
class SVE_3_Op_Pat_SelZero<ValueType vtd, SDPatternOperator op, ValueType vt1,
                   ValueType vt2, ValueType vt3, Instruction inst>
: Pat<(vtd (vtd (op vt1:$Op1, (vselect vt1:$Op1, vt2:$Op2, (SVEDup0)), vt3:$Op3))),
      (inst $Op1, $Op2, $Op3)>;

class SVE_3_Op_Pat_Shift_Imm_SelZero<ValueType vtd, SDPatternOperator op,
                                     ValueType vt1, ValueType vt2,
                                     Operand vt3, Instruction inst>
: Pat<(vtd (op vt1:$Op1, (vselect vt1:$Op1, vt2:$Op2, (SVEDup0)), (i32 (vt3:$Op3)))),
      (inst $Op1, $Op2, vt3:$Op3)>;
}

What I don't fully understand is why the complexity has to be so high, since it suggests there are multiple competing patterns and it might be useful to understand what they are. I admit that AArch64mul_p_firstOpndWithSingleUse looks a bit unusual and I'm not sure that we should be checking for explicit opcodes such as TokenFactor, etc.

In D142998#4095821, @SjoerdMeijer wrote:

I think we need at least 3 patches:

1. Instruction selection of mad for pattern add ( mul, splat_vector(C)). That is D142656.
2. New tests for this mad pattern and isel. That is this patch, D142998.

So the above only deals with mad, and not with any mla -> mad changes. That's why I suggested to strip out any mla changes out of patches 1 and 2. If you want to make changes in this area, we will follow the same approach:

3. Create a patch to precommit new test, if applicable.
4. Create a patch that implements these mla -> mad changes.

What do you think, makes sense?

In D142656#4093946, @SjoerdMeijer wrote:

and one more request, which I forgot to add earlier:

The patch aims at:

1. Try to generate multiply-accumulate instructions post-isel at more places
2. Improving the current functionality of 'mad' instruction(which only kicks in if intrinsic is used)
3. with 'mla' instruction, it has been observed at multiple places that it generates extra register moves especially if the added is constant. This patch aims to improve this.

We should be doing one thing at a time. This patch is about 'mad' instruction selection, so please remove anything related to 3. in this patch, if possible, and prepare a follow up patch for that.

@SjoerdMeijer Regarding doing the same thing at MachineCombiner stage. Even with current trunk, MLA/MLS for SVE is generated at ISel level. The line which you pointed was for scalar MADD. Also, its relatively easy to implement pattern matching at ISel rather than machine combiner for the reason that

1. I am reusing pattern for 'mla'
2. I dont need to get deep into intricate details needed at machine combiner level

@SjoerdMeijer Yes, I will add the patch with full context.

Hi All,