[09/14] Allow nested implications for extensions.
Checks
Commit Message
Certain extensions require two levels of implications. For example,
zvkng implies zvkn and zvkn implies zvkned. Enabling zvkng should also
enable zvkned.
This patch fixes this behavior.
Signed-off-by: Nathan Huckleberry <nhuck@google.com>
---
bfd/elfxx-riscv.c | 16 ++++++++++++++--
1 file changed, 14 insertions(+), 2 deletions(-)
Comments
On 6/29/23 11:18, Nathan Huckleberry via Binutils wrote:
> Certain extensions require two levels of implications. For example,
> zvkng implies zvkn and zvkn implies zvkned. Enabling zvkng should also
> enable zvkned.
>
> This patch fixes this behavior.
>
> Signed-off-by: Nathan Huckleberry <nhuck@google.com>
> ---
> bfd/elfxx-riscv.c | 16 ++++++++++++++--
> 1 file changed, 14 insertions(+), 2 deletions(-)
No problem with the actual code. I would suggest a comment explicitly
stating this only allows two levels rather than arbitrary levels of nesting.
As with the others, it needs ChangeLog and NEWS entries.
With those changes it'll be fine for the trunk.
Thanks,
jeff
On Fri, Jun 30, 2023 at 10:45 PM Jeff Law via Binutils
<binutils@sourceware.org> wrote:
>
>
>
> On 6/29/23 11:18, Nathan Huckleberry via Binutils wrote:
> > Certain extensions require two levels of implications. For example,
> > zvkng implies zvkn and zvkn implies zvkned. Enabling zvkng should also
> > enable zvkned.
> >
> > This patch fixes this behavior.
> >
> > Signed-off-by: Nathan Huckleberry <nhuck@google.com>
> > ---
> > bfd/elfxx-riscv.c | 16 ++++++++++++++--
> > 1 file changed, 14 insertions(+), 2 deletions(-)
> No problem with the actual code. I would suggest a comment explicitly
> stating this only allows two levels rather than arbitrary levels of nesting.
If I read the resulting code correctly, then arbitrary nesting should
be possible.
Whenever an implicit subset is found and added, `finished` will be set to false,
`t` will be reset to the initial value and the inner loop will terminate.
The outer loop will start over because `finished` is false.
Do I miss something?
BR
Christoph
>
> As with the others, it needs ChangeLog and NEWS entries.
>
> With those changes it'll be fine for the trunk.
>
> Thanks,
> jeff
On 6/30/23 15:53, Christoph Müllner wrote:
> On Fri, Jun 30, 2023 at 10:45 PM Jeff Law via Binutils
> <binutils@sourceware.org> wrote:
>>
>>
>>
>> On 6/29/23 11:18, Nathan Huckleberry via Binutils wrote:
>>> Certain extensions require two levels of implications. For example,
>>> zvkng implies zvkn and zvkn implies zvkned. Enabling zvkng should also
>>> enable zvkned.
>>>
>>> This patch fixes this behavior.
>>>
>>> Signed-off-by: Nathan Huckleberry <nhuck@google.com>
>>> ---
>>> bfd/elfxx-riscv.c | 16 ++++++++++++++--
>>> 1 file changed, 14 insertions(+), 2 deletions(-)
>> No problem with the actual code. I would suggest a comment explicitly
>> stating this only allows two levels rather than arbitrary levels of nesting.
>
> If I read the resulting code correctly, then arbitrary nesting should
> be possible.
> Whenever an implicit subset is found and added, `finished` will be set to false,
> `t` will be reset to the initial value and the inner loop will terminate.
> The outer loop will start over because `finished` is false.
> Do I miss something?
This is making my brain hurt, too late on a Friday afternoon to think.
I should just put it under the debugger (the downside of trying to
review code for a hands-on learner), but I don't have the time right
now. ISTM there needs to be a worklist of things we've found that we
then need to scan.
But I'll trust you on this, you're a lot more familiar with this code in
general than I am.
I wouldn't lose any sleep if you fixed formatting in this code. We're
supposed to be using GNU style. So the open curley on the IF statement
should be on its own line, indented twice (which implies the code in the
TRUE arm ought to be reindented) and the close curley should line up
with the open curley.
Jeff
> > Whenever an implicit subset is found and added, `finished` will be set to false,
> > `t` will be reset to the initial value and the inner loop will terminate.
> > The outer loop will start over because `finished` is false.
> > Do I miss something?
This is correct. The code essentially does a BFS on the implications.
Thanks,
Huck
On Sat, Jul 1, 2023 at 12:24 AM Jeff Law <jeffreyalaw@gmail.com> wrote:
>
>
>
> On 6/30/23 15:53, Christoph Müllner wrote:
> > On Fri, Jun 30, 2023 at 10:45 PM Jeff Law via Binutils
> > <binutils@sourceware.org> wrote:
> >>
> >>
> >>
> >> On 6/29/23 11:18, Nathan Huckleberry via Binutils wrote:
> >>> Certain extensions require two levels of implications. For example,
> >>> zvkng implies zvkn and zvkn implies zvkned. Enabling zvkng should also
> >>> enable zvkned.
> >>>
> >>> This patch fixes this behavior.
> >>>
> >>> Signed-off-by: Nathan Huckleberry <nhuck@google.com>
> >>> ---
> >>> bfd/elfxx-riscv.c | 16 ++++++++++++++--
> >>> 1 file changed, 14 insertions(+), 2 deletions(-)
> >> No problem with the actual code. I would suggest a comment explicitly
> >> stating this only allows two levels rather than arbitrary levels of nesting.
> >
> > If I read the resulting code correctly, then arbitrary nesting should
> > be possible.
> > Whenever an implicit subset is found and added, `finished` will be set to false,
> > `t` will be reset to the initial value and the inner loop will terminate.
> > The outer loop will start over because `finished` is false.
> > Do I miss something?
> This is making my brain hurt, too late on a Friday afternoon to think.
> I should just put it under the debugger (the downside of trying to
> review code for a hands-on learner), but I don't have the time right
> now. ISTM there needs to be a worklist of things we've found that we
> then need to scan.
>
> But I'll trust you on this, you're a lot more familiar with this code in
> general than I am.
>
> I wouldn't lose any sleep if you fixed formatting in this code. We're
> supposed to be using GNU style. So the open curley on the IF statement
> should be on its own line, indented twice (which implies the code in the
> TRUE arm ought to be reindented) and the close curley should line up
> with the open curley.
I tested all patches with check_GNU_style.sh and it only found
what you have commented here.
Since nobody has pushed this series so far, I have updated the code
and sent out a v6
which can also be found here:
https://github.com/cmuellner/binutils-gdb/tree/riscv-zvk-v6
Thanks,
Christoph
>
>
>
>
On 6/30/23 23:22, Christoph Müllner wrote:
> On Sat, Jul 1, 2023 at 12:24 AM Jeff Law <jeffreyalaw@gmail.com> wrote:
>>
>>
>>
>> On 6/30/23 15:53, Christoph Müllner wrote:
>>> On Fri, Jun 30, 2023 at 10:45 PM Jeff Law via Binutils
>>> <binutils@sourceware.org> wrote:
>>>>
>>>>
>>>>
>>>> On 6/29/23 11:18, Nathan Huckleberry via Binutils wrote:
>>>>> Certain extensions require two levels of implications. For example,
>>>>> zvkng implies zvkn and zvkn implies zvkned. Enabling zvkng should also
>>>>> enable zvkned.
>>>>>
>>>>> This patch fixes this behavior.
>>>>>
>>>>> Signed-off-by: Nathan Huckleberry <nhuck@google.com>
>>>>> ---
>>>>> bfd/elfxx-riscv.c | 16 ++++++++++++++--
>>>>> 1 file changed, 14 insertions(+), 2 deletions(-)
>>>> No problem with the actual code. I would suggest a comment explicitly
>>>> stating this only allows two levels rather than arbitrary levels of nesting.
>>>
>>> If I read the resulting code correctly, then arbitrary nesting should
>>> be possible.
>>> Whenever an implicit subset is found and added, `finished` will be set to false,
>>> `t` will be reset to the initial value and the inner loop will terminate.
>>> The outer loop will start over because `finished` is false.
>>> Do I miss something?
>> This is making my brain hurt, too late on a Friday afternoon to think.
>> I should just put it under the debugger (the downside of trying to
>> review code for a hands-on learner), but I don't have the time right
>> now. ISTM there needs to be a worklist of things we've found that we
>> then need to scan.
>>
>> But I'll trust you on this, you're a lot more familiar with this code in
>> general than I am.
>>
>> I wouldn't lose any sleep if you fixed formatting in this code. We're
>> supposed to be using GNU style. So the open curley on the IF statement
>> should be on its own line, indented twice (which implies the code in the
>> TRUE arm ought to be reindented) and the close curley should line up
>> with the open curley.
>
> I tested all patches with check_GNU_style.sh and it only found
> what you have commented here.
>
> Since nobody has pushed this series so far, I have updated the code
> and sent out a v6
> which can also be found here:
> https://github.com/cmuellner/binutils-gdb/tree/riscv-zvk-v6
Thank you! That'll be so much easier than applying things by hand!
Fetching that remote now ;-)
jeff
On 6/30/23 23:22, Christoph Müllner wrote:
> On Sat, Jul 1, 2023 at 12:24 AM Jeff Law <jeffreyalaw@gmail.com> wrote:
>>
>>
>>
>> On 6/30/23 15:53, Christoph Müllner wrote:
>>> On Fri, Jun 30, 2023 at 10:45 PM Jeff Law via Binutils
>>> <binutils@sourceware.org> wrote:
>>>>
>>>>
>>>>
>>>> On 6/29/23 11:18, Nathan Huckleberry via Binutils wrote:
>>>>> Certain extensions require two levels of implications. For example,
>>>>> zvkng implies zvkn and zvkn implies zvkned. Enabling zvkng should also
>>>>> enable zvkned.
>>>>>
>>>>> This patch fixes this behavior.
>>>>>
>>>>> Signed-off-by: Nathan Huckleberry <nhuck@google.com>
>>>>> ---
>>>>> bfd/elfxx-riscv.c | 16 ++++++++++++++--
>>>>> 1 file changed, 14 insertions(+), 2 deletions(-)
>>>> No problem with the actual code. I would suggest a comment explicitly
>>>> stating this only allows two levels rather than arbitrary levels of nesting.
>>>
>>> If I read the resulting code correctly, then arbitrary nesting should
>>> be possible.
>>> Whenever an implicit subset is found and added, `finished` will be set to false,
>>> `t` will be reset to the initial value and the inner loop will terminate.
>>> The outer loop will start over because `finished` is false.
>>> Do I miss something?
>> This is making my brain hurt, too late on a Friday afternoon to think.
>> I should just put it under the debugger (the downside of trying to
>> review code for a hands-on learner), but I don't have the time right
>> now. ISTM there needs to be a worklist of things we've found that we
>> then need to scan.
>>
>> But I'll trust you on this, you're a lot more familiar with this code in
>> general than I am.
>>
>> I wouldn't lose any sleep if you fixed formatting in this code. We're
>> supposed to be using GNU style. So the open curley on the IF statement
>> should be on its own line, indented twice (which implies the code in the
>> TRUE arm ought to be reindented) and the close curley should line up
>> with the open curley.
>
> I tested all patches with check_GNU_style.sh and it only found
> what you have commented here.
>
> Since nobody has pushed this series so far, I have updated the code
> and sent out a v6
> which can also be found here:
> https://github.com/cmuellner/binutils-gdb/tree/riscv-zvk-v6
I had to fix Nathan's email address in various commits to satisfy the
commit hooks. So while the git hashes have changed, the actual source
contents pushed to the trunk are the same.
Thanks!
jeff
On Sat, Jul 1, 2023 at 3:33 PM Jeff Law <jeffreyalaw@gmail.com> wrote:
>
>
>
> On 6/30/23 23:22, Christoph Müllner wrote:
> > On Sat, Jul 1, 2023 at 12:24 AM Jeff Law <jeffreyalaw@gmail.com> wrote:
> >>
> >>
> >>
> >> On 6/30/23 15:53, Christoph Müllner wrote:
> >>> On Fri, Jun 30, 2023 at 10:45 PM Jeff Law via Binutils
> >>> <binutils@sourceware.org> wrote:
> >>>>
> >>>>
> >>>>
> >>>> On 6/29/23 11:18, Nathan Huckleberry via Binutils wrote:
> >>>>> Certain extensions require two levels of implications. For example,
> >>>>> zvkng implies zvkn and zvkn implies zvkned. Enabling zvkng should also
> >>>>> enable zvkned.
> >>>>>
> >>>>> This patch fixes this behavior.
> >>>>>
> >>>>> Signed-off-by: Nathan Huckleberry <nhuck@google.com>
> >>>>> ---
> >>>>> bfd/elfxx-riscv.c | 16 ++++++++++++++--
> >>>>> 1 file changed, 14 insertions(+), 2 deletions(-)
> >>>> No problem with the actual code. I would suggest a comment explicitly
> >>>> stating this only allows two levels rather than arbitrary levels of nesting.
> >>>
> >>> If I read the resulting code correctly, then arbitrary nesting should
> >>> be possible.
> >>> Whenever an implicit subset is found and added, `finished` will be set to false,
> >>> `t` will be reset to the initial value and the inner loop will terminate.
> >>> The outer loop will start over because `finished` is false.
> >>> Do I miss something?
> >> This is making my brain hurt, too late on a Friday afternoon to think.
> >> I should just put it under the debugger (the downside of trying to
> >> review code for a hands-on learner), but I don't have the time right
> >> now. ISTM there needs to be a worklist of things we've found that we
> >> then need to scan.
> >>
> >> But I'll trust you on this, you're a lot more familiar with this code in
> >> general than I am.
> >>
> >> I wouldn't lose any sleep if you fixed formatting in this code. We're
> >> supposed to be using GNU style. So the open curley on the IF statement
> >> should be on its own line, indented twice (which implies the code in the
> >> TRUE arm ought to be reindented) and the close curley should line up
> >> with the open curley.
> >
> > I tested all patches with check_GNU_style.sh and it only found
> > what you have commented here.
> >
> > Since nobody has pushed this series so far, I have updated the code
> > and sent out a v6
> > which can also be found here:
> > https://github.com/cmuellner/binutils-gdb/tree/riscv-zvk-v6
> I had to fix Nathan's email address in various commits to satisfy the
> commit hooks. So while the git hashes have changed, the actual source
> contents pushed to the trunk are the same.
Sorry for missing that!
I just downloaded the emails and applied them.
I did go through all commit messages, added the ChangeLog and cleaned
them up, but I did not look at the author line.
Thanks for fixing and pushing!
On 7/2/23 14:01, Christoph Müllner wrote:
>
> Sorry for missing that!
> I just downloaded the emails and applied them.
> I did go through all commit messages, added the ChangeLog and cleaned
> them up, but I did not look at the author line.
> Thanks for fixing and pushing!
No worries. I didn't know the email addresses had been munged or that
the hooks would test for that!
Jeff
@@ -1870,15 +1870,27 @@ static void
riscv_parse_add_implicit_subsets (riscv_parse_subset_t *rps)
{
struct riscv_implicit_subset *t = riscv_implicit_subsets;
- for (; t->subset_name; t++)
+ bool finished = false;
+ while (!finished) {
+ finished = true;
+ for (; t->subset_name; t++)
{
riscv_subset_t *subset = NULL;
+ riscv_subset_t *implicit_subset = NULL;
if (riscv_lookup_subset (rps->subset_list, t->subset_name, &subset)
- && t->check_func (t->implicit_name, subset))
+ && !riscv_lookup_subset (rps->subset_list, t->implicit_name, &implicit_subset)
+ && t->check_func (t->implicit_name, subset)) {
riscv_parse_add_subset (rps, t->implicit_name,
RISCV_UNKNOWN_VERSION,
RISCV_UNKNOWN_VERSION, true);
+
+ // Restart the loop and pick up any new implications.
+ finished = false;
+ t = riscv_implicit_subsets;
+ break;
+ }
}
+ }
}
/* Check extensions conflicts. */