This is an archive of the discontinued LLVM Phabricator instance.

[SVE][codegen] Add few more tests for MUL followed by ADD/SUB (NFC)
ClosedPublic

Authored by sushgokh on Jan 31 2023, 10:45 AM.

Details

Summary

These tests will form the base for upcoming patch for generating pseudo instructions for MLA/MAD/MLS/MSB at ISel.

This is also forms base for attempt made in D142656.

Diff Detail

Event Timeline

sushgokh created this revision.Jan 31 2023, 10:45 AM
Herald added a project: Restricted Project. · View Herald Transcript
sushgokh requested review of this revision.Jan 31 2023, 10:45 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 31 2023, 10:45 AM
sushgokh edited the summary of this revision. (Show Details)Jan 31 2023, 11:04 AM
SjoerdMeijer added inline comments.Jan 31 2023, 12:19 PM
llvm/test/CodeGen/AArch64/sve-multiply-add-accumulate.ll
12 ↗(On Diff #493671)

if I am not mistaken, the other patch will change this to

; CHECK-LABEL: muladd_i64:
; CHECK:       // %bb.0:
; CHECK-NEXT:    ptrue p0.d
; CHECK-NEXT:    mov z2.d, #1 // =0x1
; CHECK-NEXT:    mad z0.d, p0/m, z1.d, z2.d
; CHECK-NEXT:    ret

(perhaps the immediate is different but that looks irrelevant to me).

This new codegen looks better, it has 1 mov less, but that is just to return the value. There is no real use of the value, which makes this move visible. Basically what I am saying is that it is unclear what the benefit is.

But either way, I will repeat my request on the other patch: let's separate things and let this be about instruction selection of 'mad'. This means that we only want to do have 'mad' tests here, and separate out the mla -> mad changes.

sushgokh added inline comments.Jan 31 2023, 12:28 PM
llvm/test/CodeGen/AArch64/sve-multiply-add-accumulate.ll
12 ↗(On Diff #493671)

I can just commit the tests for mad.

But the changes for D142656 have effect on tests that are currently generating mla. If I commit tests for mla+mad (as I have done here), comparison will be easier for D142656

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:

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

What do you think, makes sense?

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:

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

What do you think, makes sense?

I tried seperating (1,2) and (3,4) as you say above. However, mla->mad is side effect of implementing (1) and this makes seperating (1) and (4) a difficult thing unless we introduce some hacks to do so

I tried seperating (1,2) and (3,4) as you say above. However, mla->mad is side effect of implementing (1) and this makes seperating (1) and (4) a difficult thing unless we introduce some hacks to do so

Ok, I will need to understand better why we can't separate things. But I will go back to the other patch for that.

Thanks for putting these tests in a precommit patch @sushgokh!

llvm/test/CodeGen/AArch64/sve-multiply-add-accumulate.ll
1 ↗(On Diff #493671)

Perhaps it's better to just add these tests to the existing sve-int-mad-pred.ll file?

165 ↗(On Diff #493671)

I'm not sure what this is doing that is different to @muladd_i8_negativeAddend? Are you just trying to test a case where the immediate is too big to fit into the 'add'?

sushgokh added inline comments.Feb 1 2023, 3:35 AM
llvm/test/CodeGen/AArch64/sve-multiply-add-accumulate.ll
1 ↗(On Diff #493671)

Just thought better to have seperate file because sve-int-mad-pred.ll only has intrinsics. If you suggest, will add all the tests to that file

165 ↗(On Diff #493671)

In this test, unlike the other tests, immediate is realised using 2 register moves.

Matt added a subscriber: Matt.Feb 7 2023, 8:33 PM
sushgokh updated this revision to Diff 495795.Feb 8 2023, 3:36 AM

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

sushgokh retitled this revision from [SVE][codegen] Add test case for a fused multiply-add (NFC) to [SVE][codegen] Add few more tests for MUL followed by ADD/SUB (NFC).Feb 23 2023, 2:23 AM
sushgokh edited the summary of this revision. (Show Details)
sushgokh updated this revision to Diff 499778.Feb 23 2023, 2:33 AM

@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

@paulwalker-arm Any comments/suggestions on this ?

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

sushgokh added a comment.EditedMar 6 2023, 4:07 AM

@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 As agreed in last comment for D142656,

  1. Motive of this patch is converting to pseudo instructions. I was assuming these are fairly simple tests with one of opernds as shuffle. If you just expect one liners, I think tests are already present for this patch. Agreed ?

So, to reiterate the line of action,

  1. This patch will commit tests for converting to pseudo instructions
  2. Next patch will actually convert to pseudo instructions
  3. Patch for converting to FMA in D142656. I am not addressing D142656 immediately

Fair enough. Sorry for the misunderstanding.

llvm/test/CodeGen/AArch64/sve-generate-pseudo.ll
4 ↗(On Diff #499778)

Please use update_llc_test_checks.py to autogenerate the CHECK lines. Or perhaps why not just place all the tests within sve-int-arith.ll? given they're related.

10 ↗(On Diff #499778)

Please include equivalent tests for the other supports element types (i.e. i16, i32 and i64).

paulwalker-arm added inline comments.Mar 6 2023, 4:39 AM
llvm/test/CodeGen/AArch64/sve-generate-pseudo.ll
10 ↗(On Diff #499778)

I forgot to mention. Please also add equivalent tests that are expected to result in FMAD/FMSB being emitted once the follow on patch lands.

sushgokh added inline comments.Mar 6 2023, 4:42 AM
llvm/test/CodeGen/AArch64/sve-generate-pseudo.ll
4 ↗(On Diff #499778)

I am not autogenerating because we just want to check the pseudo instruction thats generated. I suppose we arent interested in other instructions. Will add tests for other types.

I have not added them to sve-int-arith.ll for reason that:

  1. Want to avoid auto-generation as said above
  2. File name clearly states the purpose
10 ↗(On Diff #499778)

agreed. Thanks

sushgokh added inline comments.Mar 6 2023, 4:48 AM
llvm/test/CodeGen/AArch64/sve-generate-pseudo.ll
10 ↗(On Diff #499778)

Current tests in sve-generate-pseudo.ll are for MAD/MSB once the follow on patch lands. i.e. currently, they are generating MLA/MLS but once the follow on patch lands, they will result in MAD/MSB being generated.

So, thanks. Will add assembly instructions as well in the check lines to indicate actual instruction generated

paulwalker-arm added inline comments.Mar 6 2023, 5:08 AM
llvm/test/CodeGen/AArch64/sve-generate-pseudo.ll
4 ↗(On Diff #499778)

The rational here does not make sense to me as the use of pseudo instructions it not revenant from a testing point of view. These tests are simply to show the expected output from a given blob of IR.

That said, after looking at sve-int-arith.ll I can see now that these are just clones of existing tests (mla_i8 and mls_i8) and so what I'm really asking for is this patch to extend those tests within sve-int-arith.ll to cover the other element types for all of mla, mad, mls, and msb.

sushgokh updated this revision to Diff 502734.Mar 6 2023, 11:02 AM
paulwalker-arm added inline comments.Mar 6 2023, 11:09 AM
llvm/test/CodeGen/AArch64/sve-int-arith.ll
402–403

Does simplify the mla tests to:

%prod = mul <vscale x 2 x i64> %b, %c
%res = add <vscale x 2 x i64> %a, %prod
ret <vscale x 2 x i64> %res

give the desired output?

paulwalker-arm accepted this revision.Mar 6 2023, 12:21 PM

A few recommended improvements but otherwise looks good.

llvm/test/CodeGen/AArch64/sve-int-arith.ll
342

Please name the tests after the expected resulting instruction, so mad in this case.

508–512

As above, I think this can be just:

%prod = mul <vscale x 8 x i16> %b, %c
%res = sub <vscale x 8 x i16> %a, %prod
ret <vscale x 8 x i16> %res
This revision is now accepted and ready to land.Mar 6 2023, 12:21 PM
sushgokh added inline comments.Mar 6 2023, 11:15 PM
llvm/test/CodeGen/AArch64/sve-int-arith.ll
342

So, I have named it as per the current instruction. Once the pseudo instr generation patch lands, will name as per appropriate instruction. Sounds good?

402–403

Yes, it works. Thanks. WIll update tests.