llvm dev - Feb 2019 - [RFC] Enforcing immediate operands for intrinsics

Hi,

I would like to solve the longstanding need for a way to indicate which
parameters to an intrinsic are required to be immediates. It should be possible
to declare in tablegen which parameters must be a trivial constant, or else the
IR is invalid.

The verifier could then reject invalid intrinsic calls, so code handling the
intrinsics doesn’t need to worry about invalid arguments. Currently any code
that deals with such intrinsics needs to do type checks on the argument to avoid
crashing on valid IR. This isn’t done particularly consistently (e.g. see
r352904, or the follow-up r353097 for a recent example fix). From the codegen
side, we do things like folding invalid intrinsic calls to undef during custom
lowering, which is more boilerplate which shouldn’t be necessary.

It’s also necessary in a few some passes to know it’s illegal to replace an
argument with a constant. llvm::canReplaceOperandWithVariable currently has to
conservatively assume any intrinsic arguments can’t be touched.

I have 2 versions of partial implementations of this.

  1.  Uses a new intrinsic query table to return a bitmask of which operands
need to be constant
  2.  Introduces a new parameter attribute

My current preference is for option 2. I initially expected to create the table,
but then I was creating an uglier way of tracking parameter properties that
exactly tracked alongside the attribute handling. It seems cleaner to just put
it there, even though it seems a bit overkill and looks slightly strange.

The rules for the attribute will look like:

  *   Only allowed on intrinsics declarations. It is not allowed on an arbitrary
function
  *   Not allowed on individual call sites
  *   The parameter must be a trivial constant leaf (i.e. ConstantInt,
ConstantFP, or Undef). No aggregates or vectors are allowed
  *   It will be incompatible with all other parameter attributes such as sret
or returned

For bikeshedding the name, I’m currently calling the attribute “constant”, but I
think this is a bad name. It doesn’t allow arbitrary constants (such as
ConstantExprs), so I think something more like “immarg” ‘or “immediate” would be
better.

-Matt
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20190205/d7265fbb/attachment.html>

Adding an "immarg" attribute makes sense; thanks for working on this.


-Eli


________________________________
From: llvm-dev <llvm-dev-bounces at lists.llvm.org> on behalf of
Arsenault, Matthew via llvm-dev <llvm-dev at lists.llvm.org>
Sent: Tuesday, February 5, 2019 5:58 AM
To: llvm-dev
Subject: [EXT] [llvm-dev] [RFC] Enforcing immediate operands for intrinsics

Hi,

I would like to solve the longstanding need for a way to indicate which
parameters to an intrinsic are required to be immediates. It should be possible
to declare in tablegen which parameters must be a trivial constant, or else the
IR is invalid.

The verifier could then reject invalid intrinsic calls, so code handling the
intrinsics doesn't need to worry about invalid arguments. Currently any code
that deals with such intrinsics needs to do type checks on the argument to avoid
crashing on valid IR. This isn't done particularly consistently (e.g. see
r352904, or the follow-up r353097 for a recent example fix). From the codegen
side, we do things like folding invalid intrinsic calls to undef during custom
lowering, which is more boilerplate which shouldn't be necessary.

It's also necessary in a few some passes to know it's illegal to replace
an argument with a constant. llvm::canReplaceOperandWithVariable currently has
to conservatively assume any intrinsic arguments can't be touched.

I have 2 versions of partial implementations of this.

  1.  Uses a new intrinsic query table to return a bitmask of which operands
need to be constant
  2.  Introduces a new parameter attribute

My current preference is for option 2. I initially expected to create the table,
but then I was creating an uglier way of tracking parameter properties that
exactly tracked alongside the attribute handling. It seems cleaner to just put
it there, even though it seems a bit overkill and looks slightly strange.

The rules for the attribute will look like:

  *   Only allowed on intrinsics declarations. It is not allowed on an arbitrary
function
  *   Not allowed on individual call sites
  *   The parameter must be a trivial constant leaf (i.e. ConstantInt,
ConstantFP, or Undef). No aggregates or vectors are allowed
  *   It will be incompatible with all other parameter attributes such as sret
or returned

For bikeshedding the name, I'm currently calling the attribute
"constant", but I think this is a bad name. It doesn't allow
arbitrary constants (such as ConstantExprs), so I think something more like
"immarg" 'or "immediate" would be better.

-Matt
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20190205/02eb11a3/attachment-0001.html>

On 2/5/19 12:55 PM, Eli Friedman via llvm-dev wrote:

Adding an "immarg" attribute makes sense; thanks for working on this.


+1


 -Hal



-Eli


________________________________
From: llvm-dev <llvm-dev-bounces at
lists.llvm.org><mailto:llvm-dev-bounces at lists.llvm.org> on behalf of
Arsenault, Matthew via llvm-dev <llvm-dev at
lists.llvm.org><mailto:llvm-dev at lists.llvm.org>
Sent: Tuesday, February 5, 2019 5:58 AM
To: llvm-dev
Subject: [EXT] [llvm-dev] [RFC] Enforcing immediate operands for intrinsics

Hi,

I would like to solve the longstanding need for a way to indicate which
parameters to an intrinsic are required to be immediates. It should be possible
to declare in tablegen which parameters must be a trivial constant, or else the
IR is invalid.

The verifier could then reject invalid intrinsic calls, so code handling the
intrinsics doesn’t need to worry about invalid arguments. Currently any code
that deals with such intrinsics needs to do type checks on the argument to avoid
crashing on valid IR. This isn’t done particularly consistently (e.g. see
r352904, or the follow-up r353097 for a recent example fix). From the codegen
side, we do things like folding invalid intrinsic calls to undef during custom
lowering, which is more boilerplate which shouldn’t be necessary.

It’s also necessary in a few some passes to know it’s illegal to replace an
argument with a constant. llvm::canReplaceOperandWithVariable currently has to
conservatively assume any intrinsic arguments can’t be touched.

I have 2 versions of partial implementations of this.

  1.  Uses a new intrinsic query table to return a bitmask of which operands
need to be constant
  2.  Introduces a new parameter attribute

My current preference is for option 2. I initially expected to create the table,
but then I was creating an uglier way of tracking parameter properties that
exactly tracked alongside the attribute handling. It seems cleaner to just put
it there, even though it seems a bit overkill and looks slightly strange.

The rules for the attribute will look like:

  *   Only allowed on intrinsics declarations. It is not allowed on an arbitrary
function
  *   Not allowed on individual call sites
  *   The parameter must be a trivial constant leaf (i.e. ConstantInt,
ConstantFP, or Undef). No aggregates or vectors are allowed
  *   It will be incompatible with all other parameter attributes such as sret
or returned

For bikeshedding the name, I’m currently calling the attribute “constant”, but I
think this is a bad name. It doesn’t allow arbitrary constants (such as
ConstantExprs), so I think something more like “immarg” ‘or “immediate” would be
better.

-Matt



_______________________________________________
LLVM Developers mailing list
llvm-dev at lists.llvm.org<mailto:llvm-dev at lists.llvm.org>
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev


--
Hal Finkel
Lead, Compiler Technology and Programming Languages
Leadership Computing Facility
Argonne National Laboratory
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20190205/7097ec58/attachment.html>

There is something similar going on in clang in SemaChecking.cpp. The 
difference is that each target can write its own verification code, 
which may check things like value ranges, for example.
The scheme you're proposing would invent a new attribute (a widespread 
change) that only implements partial checks (i.e. "is argument immediate 
or not").

Why not simply add a function to TTI that tells you whether a particular 
Value is a valid n-th argument to the given intrinsic?
Something like
   if (TTI.isValidArgument(IntOpc, Val, OpIdx))
     Int->setOperand(Val, OpIdx);

-Krzysztof


On 2/5/2019 7:58 AM, Arsenault, Matthew via llvm-dev
wrote:
> Hi,
> 
> I would like to solve the longstanding need for a way to indicate which 
> parameters to an intrinsic are required to be immediates. It should be 
> possible to declare in tablegen which parameters must be a trivial 
> constant, or else the IR is invalid.
> 
> The verifier could then reject invalid intrinsic calls, so code handling 
> the intrinsics doesn’t need to worry about invalid arguments. Currently 
> any code that deals with such intrinsics needs to do type checks on the 
> argument to avoid crashing on valid IR. This isn’t done particularly 
> consistently (e.g. see r352904, or the follow-up r353097 for a recent 
> example fix). From the codegen side, we do things like folding invalid 
> intrinsic calls to undef during custom lowering, which is more 
> boilerplate which shouldn’t be necessary.
> 
> It’s also necessary in a few some passes to know it’s illegal to replace 
> an argument with a constant. llvm::canReplaceOperandWithVariable 
> currently has to conservatively assume any intrinsic arguments can’t be 
> touched.
> 
> I have 2 versions of partial implementations of this.
> 
>  1. Uses a new intrinsic query table to return a bitmask of which
>     operands need to be constant
>  2. Introduces a new parameter attribute
> 
> My current preference is for option 2. I initially expected to create 
> the table, but then I was creating an uglier way of tracking parameter 
> properties that exactly tracked alongside the attribute handling. It 
> seems cleaner to just put it there, even though it seems a bit overkill 
> and looks slightly strange.
> 
> The rules for the attribute will look like:
> 
>   * Only allowed on intrinsics declarations. It is not allowed on an
>     arbitrary function
>   * Not allowed on individual call sites
>   * The parameter must be a trivial constant leaf (i.e. ConstantInt,
>     ConstantFP, or Undef). No aggregates or vectors are allowed
>   * It will be incompatible with all other parameter attributes such as
>     sret or returned
> 
> For bikeshedding the name, I’m currently calling the attribute 
> “constant”, but I think this is a bad name. It doesn’t allow arbitrary 
> constants (such as ConstantExprs), so I think something more like 
> “immarg” ‘or “immediate” would be better.
> 
> -Matt
> 
> 
>
<http://www.avg.com/email-signature?utm_medium=email&utm_source=link&utm_campaign=sig-email&utm_content=emailclient>
> 	Virus-free. www.avg.com 
>
<http://www.avg.com/email-signature?utm_medium=email&utm_source=link&utm_campaign=sig-email&utm_content=emailclient>
> 
> 
> <#DAB4FAD8-2DD7-40BB-A1B8-4E2AA1F9FDF2>
> 
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>

> On Feb 5, 2019, at 6:55 PM, Krzysztof Parzyszek via llvm-dev <llvm-dev
at lists.llvm.org> wrote:
> 
> There is something similar going on in clang in SemaChecking.cpp. The
difference is that each target can write its own verification code, which may
check things like value ranges, for example.
> The scheme you're proposing would invent a new attribute (a widespread
change) that only implements partial checks (i.e. "is argument immediate or
not”).
This is for the benefit of IR transform passes and codegen. I’m not attempting
to diagnose frontend builtins or do anything for the end user. A backend can
always truncate bits or whatever to fit the actual instruction constraints if
necessary. It has to do something to not crash on the IR otherwise, which is
part of the problem I’m trying to solve.
> 
> Why not simply add a function to TTI that tells you whether a particular
Value is a valid n-th argument to the given intrinsic?
> Something like
>  if (TTI.isValidArgument(IntOpc, Val, OpIdx))
>    Int->setOperand(Val, OpIdx);
> 
This is required for correctness, so TTI is not appropriate. Reasonably
implementing this would still require adding something in TableGen (which then
just brings you back to adding an attribute). I would also like to be able to
rely on this for emission of G_INTRINISIC_* instructions in GlobalISel, so that
intermediate illegal G_CONSTANT instructions can be avoided

-Matt