[llvm] [X86] Adding lowerings for vector ISD::LRINT and ISD::LLRINT (PR #90065)

[Phoebe Wang via llvm-commits]

phoebewang wrote:

> > > > > > > > > > Isn't it a poison vaule if the inputs larger than 2^32 according to [LLVM LangRef](https://llvm.org/docs/LangRef.html#trunc-to-instruction)?
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > I think only if the `trunc` would have a `nuw` flag, which isn't present in the reproducer?
> > > > > > > > 
> > > > > > > > 
> > > > > > > > @fhahn Reviewed the code again, I think it's arguable to consider it's a mis optimization. The reason is not about `trunc` with `nuw` or not, but `llvm.lrint` itself. I think we should consider the difference between `llvm.lrint.i64.f32` and `llvm.llrint.i64.f32`. The front end won't generate `llvm.lrint.i32.f32` for `lrint`, but user might expect `foo` is more optimal than `bar` when they writing
> > > > > > > > ```
> > > > > > > > int foo(float x) {
> > > > > > > >   return lrintf(x);
> > > > > > > > }
> > > > > > > > 
> > > > > > > > int bar(float x) {
> > > > > > > >   return llrintf(x);
> > > > > > > > }
> > > > > > > > ```
> > > > > > > > 
> > > > > > > > 
> > > > > > > >     
> > > > > > > >       
> > > > > > > >     
> > > > > > > > 
> > > > > > > >       
> > > > > > > >     
> > > > > > > > 
> > > > > > > >     
> > > > > > > >   
> > > > > > > > I admit we don't describe `llvm.lrint.i64.f32` in this way, but we can improve it and suggest user to use `llrintf/llvm.llrint.i64.f32` instead when they care about inputs larger than 2^32. WDYT?
> > > > > > > 
> > > > > > > 
> > > > > > > Doesn't the frontend generate llvm.lrint.i32.f32 on 32-bit targets and on 64-bit Windows? `sizeof(long)` is 4 bytes in those cases.
> > > > > > 
> > > > > > 
> > > > > > But they don't have difference on 64-bit Linux, I think it's good to expand its semantic given it's already inconsistent across different target and OS.
> > > > > 
> > > > > 
> > > > > Aren't the semantics defined by C not by LLVM IR. How can you change it?
> > > > 
> > > > 
> > > > The [LangRef](https://llvm.org/docs/LangRef.html#llvm-lrint-intrinsic) says `This function returns the same values as the libm lrint functions would, but without setting errno.` In my opinion, it is not a clear definition given:
> > > > 
> > > > * It doesn't cover the Windows behavior because it doesn't use libm;
> > > 
> > > 
> > > I think that's poor wording. I think the intent is to cover any implementation of the C math library.
> > > > * It doesn't mention the implicit inconsistent from libm;
> > > > * It doesn't cover the vector type behavior because they don't map to libm directly;
> > > > 
> > > > So I think we can improve its semantic, it's not necessarily identical with C (e.g., we already excluded errno).
> > > 
> > > 
> > > Is your proposal to tell C programmers they shouldn't use `lrint` if they care about values larger than 2^32 by changing the LLVM IR semantics? Most C programmers don't know and/or care that LLVM IR exists. Wouldn't it be a deviation from the C standard?
> > 
> > 
> > The C code won't generate `llvm.lrint` by default https://godbolt.org/z/WMroczG5r. Yes, they may still observe behavior change when using `-ffast-math`, so we need to tell them through release note or somewhere, but that's change regarding fast math rather than C standard.
> 
> Is -fno-math-errno sufficient?

Good point! If we refer to GCC's 32-bit case, we can see there is differece between `-ffast-math` and `-fno-math-errno` https://godbolt.org/z/b4oE67bGb. But Clang doesn't distinguish them so far. So it would be an independent problem to what we are discussing.

https://github.com/llvm/llvm-project/pull/90065