[PATCH] D96836: [FPEnv][AArch64] Implement lowering of llvm.set.rounding

[Serge Pavlov via Phabricator via llvm-commits]

sepavloff added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:3488
+  // Put new rounding mode into FPSCR[23:22].
+  const unsigned RMMask =
+      ~(AArch64::Rounding::rmMask << AArch64::RoundingBitsPos);
----------------
david-arm wrote:
> Should RMMask be uint64_t here since you're creating a MVT::i64 constant?
Yes, it caused inefficient code generation, as noted by @simon_tatham. How it has type `int` because it is converted to 64-bit value using sign extension, so produces convenient constant.



================
Comment at: llvm/test/CodeGen/AArch64/fpenv.ll:12
+; CHECK:       sub   w10, w0, #1
+; CHECK:       and   x8, x8, x9
+; CHECK:       and   w9, w10, #0x3
----------------
simon_tatham wrote:
> sepavloff wrote:
> > simon_tatham wrote:
> > > This may be outside the scope of your patch, but do you have any idea why this hasn't come out as `BIC x8, x8, #0xC00000`, but instead, has carefully materialized `~0xC00000` in a register so that it can use `AND` instead of `BIC`?
> > Looking at A64 instruction documentation, I see that `BIC` has only `register, register` variant.
> Yes, sorry – I misremembered what the official name of the instruction was. (I'm more of an AArch32 expert than AArch64.)
> 
> But there is an immediate instruction that does what you want. This C source
> ```unsigned long foo(unsigned long x) { return x & ~0xC00000L; }
> ```
> compiles to this instruction:
> ```        and     x0, x0, #0xffffffffff3fffff
> ```
> 
This is result of using `unsigned` for `RMMask`. When it is converted to 64-bit value, upper 32 bits were zero, this constant cannot be represented as a logical constant in AND instruction, so more complex construct was used for this constant. Using `int` as the variable type results in sign-extended value, in which upper half is all ones and it can be encoded as immediate in AND.

The generated code becomes better. Thank you!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96836