This is an archive of the discontinued LLVM Phabricator instance.

[AArch64][SVE] Add fixed length codegen for FP_ROUND/FP_EXTEND
ClosedPublic

Authored by bsmith on May 17 2021, 3:50 AM.

Diff Detail

Event Timeline

bsmith created this revision.May 17 2021, 3:50 AM
bsmith requested review of this revision.May 17 2021, 3:50 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 17 2021, 3:50 AM
Matt added a subscriber: Matt.May 17 2021, 5:08 AM
peterwaller-arm accepted this revision.May 19 2021, 7:48 AM
This revision is now accepted and ready to land.May 19 2021, 7:48 AM

I've added a recommendation that I believe will also simplify D102777.

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
17838–17861

Although correct can I recommend a different approach. Rather than "any_extending" the scalable value you should be able to actually ISD::ANY_EXTEND the fixed length operand directly. This simplifies the lowering and means the any_extend->uunplo transform is kept in one place. The flow can be something like:

src = bit cast(src_as_int, src)
src = any_extend(dst_as_int, src)
src = convertToScalable(src)
src = svesafecast(src)
src = fpextend(src)

The legaliser will then transform the any_extend separately.

17888

Although this is more direct, if you wanted to loose the line wrapping you can use changeTypeToInteger().

17891–17910

As above, by doing an actual ISD::TRUNCATE of the fixed length result you get to reuse the existing mechanism to lower fixed length truncates. I'm thinking:

Val = convertFromScalableVector(DAG, SrcVT.changeTypeToInteger(), Val);           
Val = DAG.getNode(ISD::TRUNCATE, DL, VT.changeTypeToInteger(), Val);              
return DAG.getNode(ISD::BITCAST, DL, VT, Val);
bsmith updated this revision to Diff 346688.May 20 2021, 3:44 AM
bsmith marked 3 inline comments as done.
  • Vastly improve code by re-using fixed length extends/truncates
peterwaller-arm accepted this revision.May 20 2021, 9:30 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
3218–3219

This change looks unnecessary.

17842

This can be SrcVT, which when combined with the next comment means ContainerSrcVT is no longer required.

17847

I'm surprised this works as ContainerSrcVT and Val have different element types? Either way it seems safer to use ContainerDstVT.changeTypeToInteger().

17867

This can be VT, which means ContainerDstVT is no longer needed.

llvm/test/CodeGen/AArch64/sve-fixed-length-fp-extend-trunc.ll
61–67

In general for the fixed length arith tests we've added VBITS_EQ_256 CHECK lines to the 512bit tests, just to show sensible type legalisation (see sve-fixed-length-fp-reduce.ll).

bsmith updated this revision to Diff 347023.May 21 2021, 7:23 AM
bsmith marked 5 inline comments as done.
  • Variable name cleanups
  • Add tests to check type legalisation
paulwalker-arm accepted this revision.May 21 2021, 7:52 AM
This revision was landed with ongoing or failed builds.May 24 2021, 5:03 AM
This revision was automatically updated to reflect the committed changes.