[PATCH] D96501: [FPEnv][ARM] Implement lowering of llvm.set.rounding

Fri Feb 12 02:49:05 PST 2021

sepavloff added a comment.

In D96501#2557041 <https://reviews.llvm.org/D96501#2557041>, @simon_tatham wrote:

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

================
Comment at: llvm/lib/Target/ARM/ARMISelLowering.cpp:6131
+  // a table, which consists of a sequence of 2-bit fields, each representing
+  // corresponding RISCV mode.
+  static const unsigned Table =
----------------
simon_tatham wrote:
> Corresponding Arm mode, surely!
Sure) Copy-past error.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96501/new/

https://reviews.llvm.org/D96501