Hi Segher & Peter, Thanks for your comments!!
on 2024/8/10 05:43, Segher Boessenkool wrote: > On Fri, Aug 09, 2024 at 03:50:50PM -0500, Peter Bergner wrote: >> On 8/9/24 12:54 PM, Segher Boessenkool wrote: >>>> --- a/gcc/config/rs6000/altivec.md >>>> +++ b/gcc/config/rs6000/altivec.md >>>> @@ -623,7 +623,7 @@ (define_insn "altivec_eqv1ti" >>>> [(set (match_operand:V1TI 0 "altivec_register_operand" "=v") >>>> (eq:V1TI (match_operand:V1TI 1 "altivec_register_operand" "v") >>>> (match_operand:V1TI 2 "altivec_register_operand" "v")))] >>>> - "TARGET_POWER10" >>>> + "TARGET_P10_VECTOR" >>>> "vcmpequq %0,%1,%2" >>>> [(set_attr "type" "veccmpfx")]) >>> >>> This very first one is incorrect, already. This is a Vector insn >>> (it needs MSR[VEC]=1), not a VSX insn (for which MSR[VSX]=1 is needed). >>> >>> We test TARGET_ALTIVEC for that, not TARGET_VSX. >> >> I guess you are correct that *_VECTOR is not specific enough because >> yeah, we could have -mcpu=power10 -maltivec -mno-vsx so we'd need two >> macros, TARGET_P10_ALTIVEC and TARGET_P10_VSX rather than one catch-all. > > The instructions are part of the Vector Facility. Not the Vector-Scalar > Extension Facility. There is a difference, and the two are gated by > different MSR bits. This is *fundamental*. > > Yes, often both are enabled. Often *everything* is enabled. In the > compiler we cannot rely on the happy case often. I agree with the difference between Vector Facility and Vector-Scalar Extension Facility, the proposed TARGET_P10_VECTOR followed the existing TARGET_P[89]_VECTOR usage, which guard for both VMX and VSX insns on Power[89] (it means P8/9 VMX insns are not supported even with -maltivec), I interpreted it was intentional to disable VMX with the implication VMX units actually being VSX units. But according to Segher's comments, we want to separate them, I think it means we have to rework the current TARGET_P[89]_VECTOR support first. > >>> In general, we want to get rid of TARGET_Pxxx_VECTOR, not introduce new >>> stuff like it! >> >> I'm fine with the TARGET_P10_* macro, since it's more readable than saying >> TARGET_POWER10 && TARGET_ALTIVEC && TARGET_VSX, especially when we use the >> negated version. Yes, TARGET_P[89]_VECTOR means TARGET_POWER[89] && TARGET_VSX (vsx implies altivec should be set). > > It is not more readable *at all*. What does it even mean? Previous > similar macros (TARGET_P8_VECTOR) meant that various VSX instructions > new in ISA 2.07 were enabled, *or* that some vector insns (either VMX or > VSX, it never was clear which) were enabled, and we were compiling for > 2.07 or later. It meant the former, but was often understood as meaning > the latter. It was a *mess*. We should not make a bigger mess. IIUC, we want to split TARGET_P[89]_VECTOR into TARGET_P[89]_ALTIVEC and TARGET_P[89]_VSX (or just TARGET_POWER[89] && TARGET_VSX or TARGET_ALTIVEC) according to the context (VMX or VSX), and we need to split power[89]-vector bif stanzas, isa attributes etc. One difference with this change is that previously users specify -mno-vsx to disable all vector insns (both VMX and VSX) on Power[89], now they should use -mno-altivec for that purpose. I think it's better as it matches the behaviors on Power7? > > Convenience macros are fine, but it should be clear what they MEAN! > Clear to the uninitiated. Obvious, self-explanatory. Not having two > disparate meanings, both "obvious"! > > > Segher BR, Kewen
