This is an archive of the discontinued LLVM Phabricator instance.

[AArch64] Use simd mov to materialize big fp constants
ClosedPublic

Authored by Allen on Feb 23 2022, 8:36 PM.

Diff Detail

Event Timeline

Allen created this revision.Feb 23 2022, 8:36 PM
Allen requested review of this revision.Feb 23 2022, 8:36 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 23 2022, 8:36 PM
Allen updated this revision to Diff 411074.Feb 24 2022, 4:25 AM
Allen edited the summary of this revision. (Show Details)
This comment was removed by Allen.
Allen added a comment.Feb 24 2022, 4:28 AM

fix precommit format

Using the MOVI to materialize fp constants sounds like a good idea.

But I don't think this should be starting from a CopyToReg. CopyToReg usually means we are copying the result into the return value of a function (or into a call argument). Copying into s0 for example. CopyFromReg is used to get the value from input physical registers from arguments.

An example like this, with fadd, would preferably use the movi lowering too: https://godbolt.org/z/bc99E85qz

If the FPConstant is still in the DAG at the point that we run instruction selection (I think it should be but it may be dependant on what AArch64TargetLowering::isFPImmLegal returns), then it may be best to add a tablegen pattern for cases that we can materialize with the integer movi instruction. From here: https://github.com/llvm/llvm-project/blob/ecff9b65b54c7a4bd79ca2af157c81595678f0ee/llvm/lib/Target/AArch64/AArch64InstrInfo.td#L1545, with a different fpimm to limit it to cases where the MOVi can be generated, and an EXTRACT_SUBREG to get the s reg. If that doesn't work (the custom ImmLeaf's might be a bit much) then perhaps something in AArch64ISelDAGToDAG.cpp?

Allen updated this revision to Diff 411608.Feb 26 2022, 6:59 AM
Allen retitled this revision from [AArch64] Use simd mov to initialise big const float immediate to [AArch64] Use simd mov to materialize big fp constants.
Allen added a comment.Feb 26 2022, 7:01 AM

Using the MOVI to materialize fp constants sounds like a good idea.

Thanks very much for detail, refactor with above new method.

Allen updated this revision to Diff 411714.Feb 27 2022, 5:22 PM
Allen updated this revision to Diff 411754.Feb 28 2022, 1:11 AM

update test cases

fhahn added a subscriber: fhahn.Feb 28 2022, 1:19 AM
fhahn added inline comments.
llvm/test/Transforms/LoopVectorize/AArch64/remat-const-float-simd.ll
2 ↗(On Diff #411754)

It looks like the test is completely unrelated to the loop-vectorize pass. It should probably be either moved to CodeGen/AArch64/ or added to one of the existing tests there.

dmgreen added inline comments.Feb 28 2022, 1:30 AM
llvm/lib/Target/AArch64/AArch64InstrFormats.td
1210

This likely doesn't need to be an Operand, it can just be a FPImmLeaf

llvm/lib/Target/AArch64/AArch64InstrInfo.td
1551

This doesn't need to define a new instruction, it can use the existing MOVI (which I believe is probably called MOVIv2i32?)

llvm/test/Transforms/LoopVectorize/AArch64/remat-const-float-simd.ll
1 ↗(On Diff #411754)

This test file should be in CodeGen/AArch64

Allen updated this revision to Diff 411776.Feb 28 2022, 3:24 AM
Allen updated this revision to Diff 411786.Feb 28 2022, 4:34 AM

a) move case remat-const-float-simd.ll to CodeGen/AArch64
b) use MOVIv2i32 , and delete unneeded instruction MOVI2s_ns
c) delete Operand<f32>, and just be a FPImmLeaf

Allen added a reviewer: fhahn.Feb 28 2022, 5:02 AM
Allen marked 2 inline comments as done.Mar 1 2022, 7:17 AM

a) move case remat-const-float-simd.ll to CodeGen/AArch64
b) use MOVIv2i32 , and delete unneeded instruction MOVI2s_ns
c) delete Operand<f32>, and just be a FPImmLeaf

Address all comment

llvm/lib/Target/AArch64/AArch64InstrFormats.td
1210

Yes, verified ok!

llvm/lib/Target/AArch64/AArch64InstrInfo.td
1551

It's strange that here error with the following info, as it is ok on my local linux ?

AArch64InstrInfo.td:1551:1: error: Type set is empty for each HW mode in 'MOVIv2s_ns' def MOVIv2s_ns : BaseSIMDModifiedImmVectorShift<1, 1, 0b10, V128, "movi", ".2s",

1551

Thanks, delete unneeded instruction MOVI2s_ns

Allen marked 2 inline comments as done.Mar 2 2022, 12:50 AM

ping ?

Herald added a project: Restricted Project. · View Herald TranscriptMar 2 2022, 12:50 AM

Thanks for the updates. There are other forms of MOVI that might also be useful for some fp immediates. The one we have here (shift an i8 by 24) I imagine is the most useful, but the others could be good in places too. The MOVN's too. I'm not sure what the best way to generalise this would be. As far as I can tell it needs lots of checks to the various isAdvSIMDModImmTypeXYZ methods wherever it is? This seems like a good beginning at least.

llvm/lib/Target/AArch64/AArch64InstrFormats.td
1210

It's likely worth calling this out as a "AdvSIMDModImmType4" constant somehow. Maybe call it fpimm32SIMDModImmType4? Same for the XForm.

llvm/lib/Target/AArch64/AArch64InstrInfo.td
6149

It is quite uncommon to not have NEON, but can you add a predicate for it:

let Predicates = [HasNEON] in {

It might be worth adding a run line without neon (-mattr=-neon) for the new remat test case too, to show the difference.

llvm/test/CodeGen/AArch64/remat-const-float-simd.ll
8

0x7fffffff -> 2147483648, as the 0x7fffffff gets rounded.

Allen updated this revision to Diff 412396.Mar 2 2022, 6:33 AM
Allen updated this revision to Diff 412402.Mar 2 2022, 6:46 AM
Allen marked 2 inline comments as done.

update testcase 0x7fffffff with 2147483648 as gets rounded

dmgreen accepted this revision.Mar 2 2022, 8:13 AM

Thanks. LGTM

This revision is now accepted and ready to land.Mar 2 2022, 8:13 AM
This revision was landed with ongoing or failed builds.Mar 4 2022, 8:36 AM
This revision was automatically updated to reflect the committed changes.