[V2] rs6000: Don't allow immediate value in the vsx_splat pattern [PR113950]
Checks
Context |
Check |
Description |
linaro-tcwg-bot/tcwg_gcc_build--master-arm |
success
|
Testing passed
|
linaro-tcwg-bot/tcwg_gcc_build--master-aarch64 |
success
|
Testing passed
|
linaro-tcwg-bot/tcwg_gcc_check--master-aarch64 |
success
|
Testing passed
|
linaro-tcwg-bot/tcwg_gcc_check--master-arm |
success
|
Testing passed
|
Commit Message
Hi All,
The following patch has been bootstrapped and regtested on powerpc64le-linux.
There is no immediate value splatting instruction in Power. Currently, those
values need to be stored in a register or memory. To address this issue, I
have updated the predicate for the second operand in vsx_splat to
splat_input_operand and corrected the assignment of op1 to operands[1].
These changes ensure that operand1 is stored in a register.
2024-02-26 Jeevitha Palanisamy <jeevitha@linux.ibm.com>
gcc/
PR target/113950
* config/rs6000/vsx.md (vsx_splat_<mode>): Updated the predicates
for second operand and corrected the assignment.
gcc/testsuite/
PR target/113950
* gcc.target/powerpc/pr113950.c: New testcase.
Comments
Hi!
On Tue, Feb 27, 2024 at 02:02:38AM +0530, jeevitha wrote:
> There is no immediate value splatting instruction in Power. Currently, those
> values need to be stored in a register or memory. To address this issue, I
> have updated the predicate for the second operand in vsx_splat to
> splat_input_operand and corrected the assignment of op1 to operands[1].
> These changes ensure that operand1 is stored in a register.
input_operand allows a lot of things that splat_input_operand does not,
not just immediate operands. NAK.
(For example, *all* memory is okay for input_operand, always).
I'm not saying we do not want to restrict these things, but a commit
that doesn't discuss this at all is not okay. Sorry.
Segher
On 2/27/24 6:40 AM, Segher Boessenkool wrote:
> On Tue, Feb 27, 2024 at 02:02:38AM +0530, jeevitha wrote:
>> There is no immediate value splatting instruction in Power. Currently, those
>> values need to be stored in a register or memory. To address this issue, I
>> have updated the predicate for the second operand in vsx_splat to
>> splat_input_operand and corrected the assignment of op1 to operands[1].
>> These changes ensure that operand1 is stored in a register.
>
> input_operand allows a lot of things that splat_input_operand does not,
> not just immediate operands. NAK.
>
> (For example, *all* memory is okay for input_operand, always).
>
> I'm not saying we do not want to restrict these things, but a commit
> that doesn't discuss this at all is not okay. Sorry.
So it seems you're not NAKing the use of splat_input_operand, but
just that it needs more explanation in the git log entry, correct?
Yes, input_operand accepts a lot more things than splat_input_operand
does, but the multiple define_insns this define_expand feeds, uses
gpc_reg_operand, memory_operand and splat_input_operand for their
operands[1] operand (splat_input_operand accepts reg and mem too),
so it seems to match better what the patterns will be accepting and
I always thought that using predicates that more accurately reflect
what the define_insns expect/accept lead to better code gen.
Mike, was it just an oversight to not use splat_input_operand for the
vsx_splat_<mode> expander or was input_operand a conscious decision?
If input_operand was used purposely, then we can just fall back to
the s/op1/operands[1]/ change which we already know fixes the bug.
Peter
On Tue, Feb 27, 2024 at 04:50:02PM -0600, Peter Bergner wrote:
> On 2/27/24 6:40 AM, Segher Boessenkool wrote:
> > On Tue, Feb 27, 2024 at 02:02:38AM +0530, jeevitha wrote:
> > input_operand allows a lot of things that splat_input_operand does not,
> > not just immediate operands. NAK.
> >
> > (For example, *all* memory is okay for input_operand, always).
> >
> > I'm not saying we do not want to restrict these things, but a commit
> > that doesn't discuss this at all is not okay. Sorry.
>
> So it seems you're not NAKing the use of splat_input_operand, but
> just that it needs more explanation in the git log entry, correct?
I NAK the patch. _Of course_ there needs to be *something* done, there
is a bug after all, it needs to be fixed.
But no, there are big questions about if splat_input_operand is correct
as well. This needs to be justified in the patch submission.
> Yes, input_operand accepts a lot more things than splat_input_operand
> does, but the multiple define_insns this define_expand feeds, uses
> gpc_reg_operand, memory_operand and splat_input_operand for their
> operands[1] operand (splat_input_operand accepts reg and mem too),
> so it seems to match better what the patterns will be accepting and
> I always thought that using predicates that more accurately reflect
> what the define_insns expect/accept lead to better code gen.
Still, it needs explanation why we allowed it before, but that was a
mistake, or for some reason we do not need it.
Sell your patch! :-)
> Mike, was it just an oversight to not use splat_input_operand for the
> vsx_splat_<mode> expander or was input_operand a conscious decision?
>
> If input_operand was used purposely, then we can just fall back to
> the s/op1/operands[1]/ change which we already know fixes the bug.
input_operand allows all inputs for mov insns. It isn't suitable
for any other instructions.
Segher
On 2/28/24 8:31 AM, Segher Boessenkool wrote:
> On Tue, Feb 27, 2024 at 04:50:02PM -0600, Peter Bergner wrote:
>> So it seems you're not NAKing the use of splat_input_operand, but
>> just that it needs more explanation in the git log entry, correct?
>
> I NAK the patch. _Of course_ there needs to be *something* done, there
> is a bug after all, it needs to be fixed.
>
> But no, there are big questions about if splat_input_operand is correct
> as well. This needs to be justified in the patch submission.
Ok, then Jeevitha, repost the patch with the s/op1/operands[1]/ only change.
Jeevitha has already bootstrapped and regtested that change and it does
fix the bug.
Clearly, the splat_input_operand change needs more discussion and would
be a follow-on patch...if we decide to do it at all.
Peter
On Wed, Feb 28, 2024 at 11:58:15AM -0600, Peter Bergner wrote:
> On 2/28/24 8:31 AM, Segher Boessenkool wrote:
> > On Tue, Feb 27, 2024 at 04:50:02PM -0600, Peter Bergner wrote:
> >> So it seems you're not NAKing the use of splat_input_operand, but
> >> just that it needs more explanation in the git log entry, correct?
> >
> > I NAK the patch. _Of course_ there needs to be *something* done, there
> > is a bug after all, it needs to be fixed.
> >
> > But no, there are big questions about if splat_input_operand is correct
> > as well. This needs to be justified in the patch submission.
>
> Ok, then Jeevitha, repost the patch with the s/op1/operands[1]/ only change.
> Jeevitha has already bootstrapped and regtested that change and it does
> fix the bug.
>
> Clearly, the splat_input_operand change needs more discussion and would
> be a follow-on patch...if we decide to do it at all.
It is clear that input_operand is wrong. It isn't clear that
splat_input_operand is correct though :-(
Segher
@@ -4660,14 +4660,14 @@
(define_expand "vsx_splat_<mode>"
[(set (match_operand:VSX_D 0 "vsx_register_operand")
(vec_duplicate:VSX_D
- (match_operand:<VEC_base> 1 "input_operand")))]
+ (match_operand:<VEC_base> 1 "splat_input_operand")))]
"VECTOR_MEM_VSX_P (<MODE>mode)"
{
rtx op1 = operands[1];
if (MEM_P (op1))
operands[1] = rs6000_force_indexed_or_indirect_mem (op1);
else if (!REG_P (op1))
- op1 = force_reg (<VSX_D:VEC_base>mode, op1);
+ operands[1] = force_reg (<VSX_D:VEC_base>mode, op1);
})
(define_insn "vsx_splat_<mode>_reg"
new file mode 100644
@@ -0,0 +1,25 @@
+/* PR target/113950 */
+/* { dg-do compile } */
+/* { dg-require-effective-target powerpc_vsx_ok } */
+/* { dg-options "-O1 -mdejagnu-cpu=power7" } */
+
+/* Verify we do not ICE on the following. */
+
+void abort (void);
+
+int main ()
+{
+ int i;
+ vector signed long long vsll_result, vsll_expected_result;
+ signed long long sll_arg1;
+
+ sll_arg1 = 300;
+ vsll_expected_result = (vector signed long long) {300, 300};
+ vsll_result = __builtin_vsx_splat_2di (sll_arg1);
+
+ for (i = 0; i < 2; i++)
+ if (vsll_result[i] != vsll_expected_result[i])
+ abort();
+
+ return 0;
+}