Page MenuHomePhabricator

[FPEnv][ARM] Implement lowering of llvm.set.rounding
ClosedPublic

Authored by sepavloff on Feb 11 2021, 6:55 AM.

Diff Detail

Event Timeline

sepavloff created this revision.Feb 11 2021, 6:55 AM
sepavloff requested review of this revision.Feb 11 2021, 6:55 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 11 2021, 6:55 AM

The table-lookup strategy seems like overkill to me. As far as I can see, the integer mapping required is: 0→3, 3→2. 2→1, 1→0. In other words, all four input values (that can be handled at all) are just reduced by 1, mod 4. So instead of (147 >> (value << 1)) & 3, you could compute (value - 1) & 3, surely more cheaply.

What about RoundingMode::NearestTiesToAway? There's no support for that in the FPSCR dynamic rounding-mode field, but it looks as if this patch will lower @llvm.set.rounding(i32 4) to some kind of code that silently does the wrong thing.

Perhaps that should have been error-checked by whatever generated the @llvm.set.rounding call in the first place? If so, I think at least a mention in the comments would be useful.

llvm/lib/Target/ARM/ARMISelLowering.cpp
6131

Corresponding Arm mode, surely!

sepavloff updated this revision to Diff 323277.Feb 12 2021, 2:41 AM

Optimized FPSCR bits calculation

The table-lookup strategy seems like overkill to me. As far as I can see, the integer mapping required is: 0→3, 3→2. 2→1, 1→0. In other words, all four input values (that can be handled at all) are just reduced by 1, mod 4. So instead of (147 >> (value << 1)) & 3, you could compute (value - 1) & 3, surely more cheaply.

Indeed, it provides more compact code, thank you.

What about RoundingMode::NearestTiesToAway? There's no support for that in the FPSCR dynamic rounding-mode field, but it looks as if this patch will lower @llvm.set.rounding(i32 4) to some kind of code that silently does the wrong thing.

Perhaps that should have been error-checked by whatever generated the @llvm.set.rounding call in the first place? If so, I think at least a mention in the comments would be useful.

Yes, this intrinsic is a low level entity so it is expected that some code elsewhere makes the necessary checks. Appropriate comment is added.

llvm/lib/Target/ARM/ARMISelLowering.cpp
6131

Sure) Copy-past error.

simon_tatham accepted this revision.Feb 12 2021, 2:55 AM

LGTM. Thanks!

This revision is now accepted and ready to land.Feb 12 2021, 2:55 AM
This revision was landed with ongoing or failed builds.Feb 12 2021, 8:17 PM
This revision was automatically updated to reflect the committed changes.
craig.topper added inline comments.
llvm/lib/Target/ARM/ARMISelLowering.cpp
6148

Intrinsic IDs should use getTargetConstant I think, but it looks like ARM is already inconsistent on this. Not sure about other targets.

sepavloff added inline comments.Feb 12 2021, 8:39 PM
llvm/lib/Target/ARM/ARMISelLowering.cpp
6148

AArch64 is inconsistent as well.