Page MenuHomePhabricator

[AArch64] Lowering and legalization of strict FP16
ClosedPublic

Authored by john.brawn on Dec 13 2021, 3:26 AM.

Details

Summary

For strict FP16 to work correctly needs some changes in lowering and legalization:

  • SelectionDAGLegalize::PromoteNode was missing handling for some strict fp opcodes.
  • Some of the custom lowering of strict fp operations needed to be adjusted to work with FP16.
  • Custom lowering needed to be added for round-to-int operations.

With this, and the previous patches for the rest of the strict fp isel, we can set IsStrictFPEnabled = true.

Diff Detail

Event Timeline

john.brawn created this revision.Dec 13 2021, 3:26 AM
john.brawn requested review of this revision.Dec 13 2021, 3:26 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 13 2021, 3:26 AM
Matt added a subscriber: Matt.Jan 4 2022, 9:38 AM
john.brawn planned changes to this revision.Jan 12 2022, 8:04 AM

I need to look at and un-xfail test/CodeGen/aarch64-v8.2a-fp16-intrinsics-constrained.c.

john.brawn edited the summary of this revision. (Show Details)

Adjusted the aarch64-v8.2a-fp16-intrinsics-constrained.c test so it can be un-XFAILed, and also adjusted some formatting.

dmgreen added inline comments.Jan 27 2022, 2:32 PM
clang/test/CodeGen/aarch64-v8.2a-fp16-intrinsics-constrained.c
11 ↗(On Diff #402546)

I am surprised to see clang tests that generate assembly directly. Is that how any other strict-fp tests work? It is usually discouraged.

john.brawn added inline comments.Feb 24 2022, 10:07 AM
clang/test/CodeGen/aarch64-v8.2a-fp16-intrinsics-constrained.c
11 ↗(On Diff #402546)

The various xyz-constrained.c tests (including non-aarch64 ones) do this.

Rebased, and also adjusted formatting slightly.

Herald added a project: Restricted Project. · View Herald TranscriptMar 7 2022, 6:04 AM
john.brawn set the repository for this revision to rG LLVM Github Monorepo.

Rebase and ping.

Herald added a project: Restricted Project. · View Herald TranscriptMar 17 2022, 5:00 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Adjusted formatting slightly.

I might have misunderstood which this patch was. Can you move the clang test into D118259 with the other? It seems like the same problem, and it looks like there should be enough llc tests to cover all the cases.

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
1383

What are the ramifications of setting this to true?

Removed clang test changes.

john.brawn added inline comments.Apr 12 2022, 9:24 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
1383

The behaviour with IsStrictFPEnabled = false is that strict fp nodes are mutated into their non-strict equivalent at the start of ISel (in SelectionDAGISel::DoInstructionSelection). Setting it to true means that this mutation doesn't happen, so strict nodes are preserved all the way through to the actual instruction selection.

dmgreen accepted this revision.Apr 14 2022, 6:29 AM

OK Thanks. This LGTM then.

This revision is now accepted and ready to land.Apr 14 2022, 6:29 AM
This revision was landed with ongoing or failed builds.Apr 14 2022, 8:51 AM
This revision was automatically updated to reflect the committed changes.