llvm dev - Jan 2020 - [RFC] Upstream development of support for yet-to-be-ratified RISC-V extensions

On Tue, 21 Jan 2020 at 01:14, Chris Lattner <clattner at nondot.org>
wrote:
>
> On Jan 16, 2020, at 10:01 AM, Alex Bradbury via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
> > I believe code should be committed to LLVM when it is of sufficient
> > quality, when it can be shown to benefit the LLVM user or developer
> > communities, and when there is someone willing to support it. All of
these can
> > be true even for unratified standard RISC-V extensions, with a caveat
on
> > "support”.
>
> +1
>
> > # Proposal
> >
> > Although we want to discourage downstream reliance on unratified
extensions,
> > there doesn't seem to be a strong motivation for requiring a
custom LLVM build
> > to force this. However, such unratified extensions shouldn't be
accessible via
> > normal RISC-V ISA naming strings (e.g. "rv32imafdc"), and
instead flags of the
> > form `-riscv-experimental-vector-ext` in LLVM and
`-mexperimental-vector-ext`
> > in Clang should be used (i.e. option 3)). We discussed this in our
weekly call
> > however, and there were voices advocating either option 2 or 3. Input
welcome.
> >
> > If going down the option 3 route, the flags could be made more
discouraging
> > through requiring an additional `-riscv-enable-experimental-extension`
flag or
> > making the flag itself longer and scarier. Thoughts?
>
> I think you’ve got the right tradeoff here.  If I’m understanding your
plan, it is:
>
>  - don’t add extensions to the isa naming string until they are finalized.
>
>  - in-progress extensions can land in mainline LLVM, and are protected
under a -experimental-foo style of flag.  There is no support implied
> or compatibility in the future for such flags, they can be changed or
removed at any time, they are just for staging.
>
>  - it doesn’t seem like there is any need to #ifdef out these flags.  A
scary enough flag name should be fine.  The goal isn’t to prevent malice, it is
to prevent accidental reliance.
>
> This all makes sense to me.
That's correct, thanks for the feedback.

I do like the idea from James of having the compiler always spit out a
note when enabling the experimental extension, warning of its
experimental nature. If we had such a warning and additionally
required a `-riscv-enable-experimental-extensions` or similar, then I
think there could be merit in including in the ISA string as Simon
suggests, especially as we're likely to start putting that string in
ELF output etc.

So based on feedback so far, I'd like to narrow the field to:

Option 1)
* Unratified extensions are not available through the ISA naming
string and are enabled via `-riscv-experimental-foo` or similar
* A warning is always emitted when enabling experimental extensions
* Support is always compiled in, and the flags aren't ifdeffed out

Option 2)
* Unratified extensions can be enabled by passing a flag like
-riscv-enable-experimental-extensions and additionally putting the
extension name and version number in the ISA naming string (version
number is always required, and we will only accept the 'current'
version number).
* A warning is always emitted when enabling experimental extensions
* Support is always compiled in, and the flags aren't ifdeffed out

Best,

Alex

On Tue, Jan 21, 2020, 05:01 Alex Bradbury via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> So based on feedback so far, I'd like to narrow the field to:
>
> Option 1)
> * Unratified extensions are not available through the ISA naming
> string and are enabled via `-riscv-experimental-foo` or similar
> * A warning is always emitted when enabling experimental extensions
> * Support is always compiled in, and the flags aren't ifdeffed out
>
> Option 2)
> * Unratified extensions can be enabled by passing a flag like
> -riscv-enable-experimental-extensions and additionally putting the
> extension name and version number in the ISA naming string (version
> number is always required, and we will only accept the 'current'
> version number).
> * A warning is always emitted when enabling experimental extensions
> * Support is always compiled in, and the flags aren't ifdeffed out

Option 2 for me!

Jacob
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200121/96308a05/attachment.html>

> On Jan 21, 2020, at 7:00, Alex Bradbury via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
> 
> ...
> 
> So based on feedback so far, I'd like to narrow the field to:
> 
> Option 1)
> * Unratified extensions are not available through the ISA naming
> string and are enabled via `-riscv-experimental-foo` or similar
> * A warning is always emitted when enabling experimental extensions
> * Support is always compiled in, and the flags aren't ifdeffed out
> 
> Option 2)
> * Unratified extensions can be enabled by passing a flag like
> -riscv-enable-experimental-extensions and additionally putting the
> extension name and version number in the ISA naming string (version
> number is always required, and we will only accept the 'current'
> version number).
> * A warning is always emitted when enabling experimental extensions
> * Support is always compiled in, and the flags aren't ifdeffed out
Option 2 seems more intentional to me, so it gets my vote.

Thank you, Alex.

__ 
Evandro Menezes ◊ SiFive ◊ Austin, TX

> So based on feedback so far, I'd like to narrow the field to:
>
> Option 1)
> * Unratified extensions are not available through the ISA naming
> string and are enabled via `-riscv-experimental-foo` or similar
> * A warning is always emitted when enabling experimental extensions
> * Support is always compiled in, and the flags aren't ifdeffed out
>
> Option 2)
> * Unratified extensions can be enabled by passing a flag like
> -riscv-enable-experimental-extensions and additionally putting the
> extension name and version number in the ISA naming string (version
> number is always required, and we will only accept the 'current'
> version number).
> * A warning is always emitted when enabling experimental extensions
> * Support is always compiled in, and the flags aren't ifdeffed out
Option 2 is what I would opt for.

- Simon

On Jan 21, 2020, at 5:00 AM, Alex Bradbury <asb at lowrisc.org>
wrote:
>> This all makes sense to me.
> 
> That's correct, thanks for the feedback.
> 
> I do like the idea from James of having the compiler always spit out a
> note when enabling the experimental extension, warning of its
> experimental nature. If we had such a warning and additionally
> required a `-riscv-enable-experimental-extensions` or similar, then I
> think there could be merit in including in the ISA string as Simon
> suggests, especially as we're likely to start putting that string in
> ELF output etc.
Are you suggesting this behavior from Clang or from LLVM?  I think it would be a
bad thing for LLVM to produce this warning: there isn’t a precedent for this,
and it breaks the library-based design goals.  Having clang produce a warning
could be done, but it would be very noisy (one warning for every .c file in a
build) and I’m not sure how much value it provides.

-Chris

Yeah. I didn't mean to actually suggest that the compiler emit a warning
diagnostic when you pass the flag. I only meant that an appropriately named
flag would be, itself, a warning sign against inappropriate use, simply by
its name, and the user having to explicitly pass it.

On Wed, Jan 22, 2020 at 2:55 PM Chris Lattner via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> On Jan 21, 2020, at 5:00 AM, Alex Bradbury <asb at lowrisc.org>
wrote:
> >> This all makes sense to me.
> >
> > That's correct, thanks for the feedback.
> >
> > I do like the idea from James of having the compiler always spit out a
> > note when enabling the experimental extension, warning of its
> > experimental nature. If we had such a warning and additionally
> > required a `-riscv-enable-experimental-extensions` or similar, then I
> > think there could be merit in including in the ISA string as Simon
> > suggests, especially as we're likely to start putting that string
in
> > ELF output etc.
>
> Are you suggesting this behavior from Clang or from LLVM?  I think it
> would be a bad thing for LLVM to produce this warning: there isn’t a
> precedent for this, and it breaks the library-based design goals.  Having
> clang produce a warning could be done, but it would be very noisy (one
> warning for every .c file in a build) and I’m not sure how much value it
> provides.
>
> -Chris
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200122/51713c24/attachment.html>

On Wed, 22 Jan 2020 at 19:55, Chris Lattner via llvm-dev
<llvm-dev at lists.llvm.org> wrote:
>
> On Jan 21, 2020, at 5:00 AM, Alex Bradbury <asb at lowrisc.org>
wrote:
> >> This all makes sense to me.
> >
> > That's correct, thanks for the feedback.
> >
> > I do like the idea from James of having the compiler always spit out a
> > note when enabling the experimental extension, warning of its
> > experimental nature. If we had such a warning and additionally
> > required a `-riscv-enable-experimental-extensions` or similar, then I
> > think there could be merit in including in the ISA string as Simon
> > suggests, especially as we're likely to start putting that string
in
> > ELF output etc.
>
> Are you suggesting this behavior from Clang or from LLVM?  I think it would
be a bad thing for LLVM to produce this warning: there isn’t a precedent for
this, and it breaks the library-based design goals.  Having clang produce a
warning could be done, but it would be very noisy (one warning for every .c file
in a build) and I’m not sure how much value it provides.
That's a good point. It felt like there may be an opportunity to
educate users that they're enabling a feature that might mutate from
release to release, but hopefully the "experimental" string in the
flag name indicates that, and as you say there's not much precedent
for such noisy warnings. After all, you can have a really bad time by
setting -mstack-alignment and not understanding the consequences.

So I'm in favour of dropping the noisy warning idea.

Thanks again for the input, and thanks James for your clarification.

Best,

Alex