diff mbox series

RISC-V: Don't make Ztso imply A

Message ID 20231213035405.2118-1-palmer@rivosinc.com
State Repeat Merge
Headers
Series RISC-V: Don't make Ztso imply A |

Checks

Context Check Description
snail/gcc-patch-check warning Git am fail log

Commit Message

Palmer Dabbelt Dec. 13, 2023, 3:54 a.m. UTC 
  I can't actually find anything in the ISA manual that makes Ztso imply
A.  In theory the memory ordering is just a different thing that the set
of availiable instructions (ie, Ztso without A would still imply TSO for
loads and stores).  It also seems like a configuration that could be
sane to build: without A it's all but impossible to write any meaningful
multi-core code, and TSO is really cheap for a single core.

That said, I think it's kind of reasonable to provide A to users asking
for Ztso.  So maybe even if this was a mistake it's the right thing to
do?

gcc/ChangeLog:

	* common/config/riscv/riscv-common.cc (riscv_implied_info):
	Remove {"ztso", "a"}.
---
 gcc/common/config/riscv/riscv-common.cc | 2 --
 1 file changed, 2 deletions(-)

Comments

Jeff Law Dec. 15, 2023, 9:37 p.m. UTC | #1 
On 12/12/23 20:54, Palmer Dabbelt wrote:
> I can't actually find anything in the ISA manual that makes Ztso imply
> A.  In theory the memory ordering is just a different thing that the set
> of availiable instructions (ie, Ztso without A would still imply TSO for
> loads and stores).  It also seems like a configuration that could be
> sane to build: without A it's all but impossible to write any meaningful
> multi-core code, and TSO is really cheap for a single core.
> 
> That said, I think it's kind of reasonable to provide A to users asking
> for Ztso.  So maybe even if this was a mistake it's the right thing to
> do?
> 
> gcc/ChangeLog:
> 
> 	* common/config/riscv/riscv-common.cc (riscv_implied_info):
> 	Remove {"ztso", "a"}.
I'd tend to think step #1 is to determine what the ISA intent is, 
meaning engagement with RVI.

We've got time for that engagement and to adjust based on the result. 
So I'd tend to defer until we know if Ztso should imply A or not.

jeff
Andrew Waterman Dec. 16, 2023, 12:14 a.m. UTC | #2 
On Fri, Dec 15, 2023 at 1:38 PM Jeff Law <jeffreyalaw@gmail.com> wrote:
>
>
>
> On 12/12/23 20:54, Palmer Dabbelt wrote:
> > I can't actually find anything in the ISA manual that makes Ztso imply
> > A.  In theory the memory ordering is just a different thing that the set
> > of availiable instructions (ie, Ztso without A would still imply TSO for
> > loads and stores).  It also seems like a configuration that could be
> > sane to build: without A it's all but impossible to write any meaningful
> > multi-core code, and TSO is really cheap for a single core.
> >
> > That said, I think it's kind of reasonable to provide A to users asking
> > for Ztso.  So maybe even if this was a mistake it's the right thing to
> > do?
> >
> > gcc/ChangeLog:
> >
> >       * common/config/riscv/riscv-common.cc (riscv_implied_info):
> >       Remove {"ztso", "a"}.
> I'd tend to think step #1 is to determine what the ISA intent is,
> meaning engagement with RVI.
>
> We've got time for that engagement and to adjust based on the result.
> So I'd tend to defer until we know if Ztso should imply A or not.

Palmer is correct.  There is no coupling between Ztso and A.  (And
there are uncontrived examples of such systems: e.g. embedded
processors without caches that don't support the LR/SC instructions,
but happen to be TSO.)

>
> jeff
Jeff Law Dec. 16, 2023, 6:58 p.m. UTC | #3 
On 12/15/23 17:14, Andrew Waterman wrote:
> On Fri, Dec 15, 2023 at 1:38 PM Jeff Law <jeffreyalaw@gmail.com> wrote:
>>
>>
>>
>> On 12/12/23 20:54, Palmer Dabbelt wrote:
>>> I can't actually find anything in the ISA manual that makes Ztso imply
>>> A.  In theory the memory ordering is just a different thing that the set
>>> of availiable instructions (ie, Ztso without A would still imply TSO for
>>> loads and stores).  It also seems like a configuration that could be
>>> sane to build: without A it's all but impossible to write any meaningful
>>> multi-core code, and TSO is really cheap for a single core.
>>>
>>> That said, I think it's kind of reasonable to provide A to users asking
>>> for Ztso.  So maybe even if this was a mistake it's the right thing to
>>> do?
>>>
>>> gcc/ChangeLog:
>>>
>>>        * common/config/riscv/riscv-common.cc (riscv_implied_info):
>>>        Remove {"ztso", "a"}.
>> I'd tend to think step #1 is to determine what the ISA intent is,
>> meaning engagement with RVI.
>>
>> We've got time for that engagement and to adjust based on the result.
>> So I'd tend to defer until we know if Ztso should imply A or not.
> 
> Palmer is correct.  There is no coupling between Ztso and A.  (And
> there are uncontrived examples of such systems: e.g. embedded
> processors without caches that don't support the LR/SC instructions,
> but happen to be TSO.)
Thanks for the confirmation.  Palmer, commit whenever is convenient for you.

jeff
Patrick O'Neill Jan. 25, 2024, 12:07 a.m. UTC | #4 
On 12/16/23 10:58, Jeff Law wrote:
>
>
> On 12/15/23 17:14, Andrew Waterman wrote:
>> On Fri, Dec 15, 2023 at 1:38 PM Jeff Law <jeffreyalaw@gmail.com> wrote:
>>>
>>>
>>>
>>> On 12/12/23 20:54, Palmer Dabbelt wrote:
>>>> I can't actually find anything in the ISA manual that makes Ztso imply
>>>> A.  In theory the memory ordering is just a different thing that 
>>>> the set
>>>> of availiable instructions (ie, Ztso without A would still imply 
>>>> TSO for
>>>> loads and stores).  It also seems like a configuration that could be
>>>> sane to build: without A it's all but impossible to write any 
>>>> meaningful
>>>> multi-core code, and TSO is really cheap for a single core.
>>>>
>>>> That said, I think it's kind of reasonable to provide A to users 
>>>> asking
>>>> for Ztso.  So maybe even if this was a mistake it's the right thing to
>>>> do?
>>>>
>>>> gcc/ChangeLog:
>>>>
>>>>        * common/config/riscv/riscv-common.cc (riscv_implied_info):
>>>>        Remove {"ztso", "a"}.
>>> I'd tend to think step #1 is to determine what the ISA intent is,
>>> meaning engagement with RVI.
>>>
>>> We've got time for that engagement and to adjust based on the result.
>>> So I'd tend to defer until we know if Ztso should imply A or not.
>>
>> Palmer is correct.  There is no coupling between Ztso and A. (And
>> there are uncontrived examples of such systems: e.g. embedded
>> processors without caches that don't support the LR/SC instructions,
>> but happen to be TSO.)
> Thanks for the confirmation.  Palmer, commit whenever is convenient 
> for you.
>
> jeff

I was going to commit on behalf of Palmer and saw this was marked as 
Deferred in patchworks:
https://patchwork.sourceware.org/project/gcc/patch/20231213035405.2118-1-palmer@rivosinc.com/

Is this an old marking from before Andrew confirmed that they are 
independent?

Thanks,
Patrick
Jeff Law Jan. 25, 2024, 12:19 a.m. UTC | #5 
On 1/24/24 17:07, Patrick O'Neill wrote:
> On 12/16/23 10:58, Jeff Law wrote:
>>
>>
>> On 12/15/23 17:14, Andrew Waterman wrote:
>>> On Fri, Dec 15, 2023 at 1:38 PM Jeff Law <jeffreyalaw@gmail.com> wrote:
>>>>
>>>>
>>>>
>>>> On 12/12/23 20:54, Palmer Dabbelt wrote:
>>>>> I can't actually find anything in the ISA manual that makes Ztso imply
>>>>> A.  In theory the memory ordering is just a different thing that 
>>>>> the set
>>>>> of availiable instructions (ie, Ztso without A would still imply 
>>>>> TSO for
>>>>> loads and stores).  It also seems like a configuration that could be
>>>>> sane to build: without A it's all but impossible to write any 
>>>>> meaningful
>>>>> multi-core code, and TSO is really cheap for a single core.
>>>>>
>>>>> That said, I think it's kind of reasonable to provide A to users 
>>>>> asking
>>>>> for Ztso.  So maybe even if this was a mistake it's the right thing to
>>>>> do?
>>>>>
>>>>> gcc/ChangeLog:
>>>>>
>>>>>        * common/config/riscv/riscv-common.cc (riscv_implied_info):
>>>>>        Remove {"ztso", "a"}.
>>>> I'd tend to think step #1 is to determine what the ISA intent is,
>>>> meaning engagement with RVI.
>>>>
>>>> We've got time for that engagement and to adjust based on the result.
>>>> So I'd tend to defer until we know if Ztso should imply A or not.
>>>
>>> Palmer is correct.  There is no coupling between Ztso and A. (And
>>> there are uncontrived examples of such systems: e.g. embedded
>>> processors without caches that don't support the LR/SC instructions,
>>> but happen to be TSO.)
>> Thanks for the confirmation.  Palmer, commit whenever is convenient 
>> for you.
>>
>> jeff
> 
> I was going to commit on behalf of Palmer and saw this was marked as 
> Deferred in patchworks:
> https://patchwork.sourceware.org/project/gcc/patch/20231213035405.2118-1-palmer@rivosinc.com/
> 
> Is this an old marking from before Andrew confirmed that they are 
> independent?
Yea, I put into deferred before Andrew chimed in.

Jeff
Palmer Dabbelt Jan. 25, 2024, 12:20 a.m. UTC | #6 
On Wed, 24 Jan 2024 16:19:06 PST (-0800), jeffreyalaw@gmail.com wrote:
>
>
> On 1/24/24 17:07, Patrick O'Neill wrote:
>> On 12/16/23 10:58, Jeff Law wrote:
>>>
>>>
>>> On 12/15/23 17:14, Andrew Waterman wrote:
>>>> On Fri, Dec 15, 2023 at 1:38 PM Jeff Law <jeffreyalaw@gmail.com> wrote:
>>>>>
>>>>>
>>>>>
>>>>> On 12/12/23 20:54, Palmer Dabbelt wrote:
>>>>>> I can't actually find anything in the ISA manual that makes Ztso imply
>>>>>> A.  In theory the memory ordering is just a different thing that
>>>>>> the set
>>>>>> of availiable instructions (ie, Ztso without A would still imply
>>>>>> TSO for
>>>>>> loads and stores).  It also seems like a configuration that could be
>>>>>> sane to build: without A it's all but impossible to write any
>>>>>> meaningful
>>>>>> multi-core code, and TSO is really cheap for a single core.
>>>>>>
>>>>>> That said, I think it's kind of reasonable to provide A to users
>>>>>> asking
>>>>>> for Ztso.  So maybe even if this was a mistake it's the right thing to
>>>>>> do?
>>>>>>
>>>>>> gcc/ChangeLog:
>>>>>>
>>>>>>        * common/config/riscv/riscv-common.cc (riscv_implied_info):
>>>>>>        Remove {"ztso", "a"}.
>>>>> I'd tend to think step #1 is to determine what the ISA intent is,
>>>>> meaning engagement with RVI.
>>>>>
>>>>> We've got time for that engagement and to adjust based on the result.
>>>>> So I'd tend to defer until we know if Ztso should imply A or not.
>>>>
>>>> Palmer is correct.  There is no coupling between Ztso and A. (And
>>>> there are uncontrived examples of such systems: e.g. embedded
>>>> processors without caches that don't support the LR/SC instructions,
>>>> but happen to be TSO.)
>>> Thanks for the confirmation.  Palmer, commit whenever is convenient
>>> for you.
>>>
>>> jeff
>>
>> I was going to commit on behalf of Palmer and saw this was marked as
>> Deferred in patchworks:
>> https://patchwork.sourceware.org/project/gcc/patch/20231213035405.2118-1-palmer@rivosinc.com/
>>
>> Is this an old marking from before Andrew confirmed that they are
>> independent?
> Yea, I put into deferred before Andrew chimed in.

OK, so I think we can just commit it?
Patrick O'Neill Jan. 25, 2024, 12:38 a.m. UTC | #7 
On 1/24/24 16:20, Palmer Dabbelt wrote:
> On Wed, 24 Jan 2024 16:19:06 PST (-0800), jeffreyalaw@gmail.com wrote:
>>
>>
>> On 1/24/24 17:07, Patrick O'Neill wrote:
>>> On 12/16/23 10:58, Jeff Law wrote:
>>>>
>>>>
>>>> On 12/15/23 17:14, Andrew Waterman wrote:
>>>>> On Fri, Dec 15, 2023 at 1:38 PM Jeff Law <jeffreyalaw@gmail.com> 
>>>>> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 12/12/23 20:54, Palmer Dabbelt wrote:
>>>>>>> I can't actually find anything in the ISA manual that makes Ztso 
>>>>>>> imply
>>>>>>> A.  In theory the memory ordering is just a different thing that
>>>>>>> the set
>>>>>>> of availiable instructions (ie, Ztso without A would still imply
>>>>>>> TSO for
>>>>>>> loads and stores).  It also seems like a configuration that 
>>>>>>> could be
>>>>>>> sane to build: without A it's all but impossible to write any
>>>>>>> meaningful
>>>>>>> multi-core code, and TSO is really cheap for a single core.
>>>>>>>
>>>>>>> That said, I think it's kind of reasonable to provide A to users
>>>>>>> asking
>>>>>>> for Ztso.  So maybe even if this was a mistake it's the right 
>>>>>>> thing to
>>>>>>> do?
>>>>>>>
>>>>>>> gcc/ChangeLog:
>>>>>>>
>>>>>>>        * common/config/riscv/riscv-common.cc (riscv_implied_info):
>>>>>>>        Remove {"ztso", "a"}.
>>>>>> I'd tend to think step #1 is to determine what the ISA intent is,
>>>>>> meaning engagement with RVI.
>>>>>>
>>>>>> We've got time for that engagement and to adjust based on the 
>>>>>> result.
>>>>>> So I'd tend to defer until we know if Ztso should imply A or not.
>>>>>
>>>>> Palmer is correct.  There is no coupling between Ztso and A. (And
>>>>> there are uncontrived examples of such systems: e.g. embedded
>>>>> processors without caches that don't support the LR/SC instructions,
>>>>> but happen to be TSO.)
>>>> Thanks for the confirmation.  Palmer, commit whenever is convenient
>>>> for you.
>>>>
>>>> jeff
>>>
>>> I was going to commit on behalf of Palmer and saw this was marked as
>>> Deferred in patchworks:
>>> https://patchwork.sourceware.org/project/gcc/patch/20231213035405.2118-1-palmer@rivosinc.com/ 
>>>
>>>
>>> Is this an old marking from before Andrew confirmed that they are
>>> independent?
>> Yea, I put into deferred before Andrew chimed in.
>
> OK, so I think we can just commit it?

Committed.

patrick
diff mbox series

Patch

diff --git a/gcc/common/config/riscv/riscv-common.cc b/gcc/common/config/riscv/riscv-common.cc
index f142212f2ed..5f39e5ea462 100644
--- a/gcc/common/config/riscv/riscv-common.cc
+++ b/gcc/common/config/riscv/riscv-common.cc
@@ -71,8 +71,6 @@  static const riscv_implied_info_t riscv_implied_info[] =
   {"zks", "zksed"},
   {"zks", "zksh"},
 
-  {"ztso", "a"},
-
   {"v", "zvl128b"},
   {"v", "zve64d"},