chandlerc wrote:

> > * Systematically using `Oi` instead of `LLi` for the type `long long int`. 
> > The `.def` file uses a mixture of `Oi` and `LLi`. I chose the shorter 
> > encoding.
> 
> The mixture use of `Oi` and `LLi` is a mess, but `Oi` has different meaning 
> for OpenCL targets. I think we should not change `LLi` to `Oi`. I think a lot 
> `Oi` can be replaced with `LLi`, but I cannot tell which are required with a 
> quick look.

I do understand that, but I'm actually a bit more confident that these changes 
are correct... Or at least not a regression.

Specifically, the x86 intrinsic builtins use `Oi` *very* consistently for "quad 
word" (64-bit) vector operations from SSE through AVX2. For example 
`__builtin_ia32_psllqi256` uses `V4Oi`. This seems quite intentional, so I was 
very hesitant to reverse it.

The places where `LLi` has crept in are:
- 1 or 2 very isolated cases I'll list explicitly below
- Some AVX-512 and SHA512 intrinsics. This seems like a mistake as they're also 
using it for "double word" (64-bit) vector elements, the exact same element 
size that uses `Oi` elsewhere. And some actually *do* use `Oi` with the same 
feature (`avx10.2-256`), so I couldn't see any pattern that seemed to indicate 
a critical distinction...

I think the AVX-512 stuff was just added without realizing the historical use 
of `Oi` here?

The only examples outside of AVX-512 and SHA512 I could find:
- `__builtin_ia32_rdpru`
- `__emul` and `__emulu` (MS intrinsics)
- `__readfsqword` and `__readgsqword` (also MS intrinsics)

I can understand that these don't make lots of sense in OpenCL, but they also 
seem very unlikely to show up or do the wrong thing here, when getting the same 
type that OpenCL uses for 64-bit vector elements on x86?

The reason I'd like to consolidate here is that preserving this distinction 
would add complexity to the tablegen code, and at least *seems* like it may be 
propagating more of a mistake than a careful choice in AVX-512 (especially 
given the lack of test coverage that errors with the change as is).

https://github.com/llvm/llvm-project/pull/120831
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to