[llvm-commits] [patch][pr12251] Add range metadata to llvm. Make clang produce it.

[Duncan Sands]

Hi Rafael,

> The attached llvm patch adds documentation and verification for the
> range metadata. I will send a patch with optimzation and analysis once
> this one is in.

> --- a/docs/LangRef.html
> +++ b/docs/LangRef.html
> @@ -3028,6 +3029,32 @@ call void @llvm.dbg.value(metadata !24, i64 0, metadata !25)
>  </pre>
>  </div>
>
> +<!-- _______________________________________________________________________ -->
> +<h4>
> +  <a name="range">'<tt>range</tt>' Metadata</a>
> +</h4>
> +
> +<div>
> +<p><tt>range</tt> metadata may be attached only to loads of integer types. It
> +   expresses the possible ranges the loaded value is in. The node is just
> +   a list of ranges,

you forgot to say what multiple ranges means.  Presumably it means that the set
of possible values is the union of the ranges, rather than the intersection or
something else - anyway you should be explicit about that.

  each range is a pair of integers with the following
> +   restrictions:</p>

restrictions -> properties (the second two lines below aren't restrictions).

> +<ul>
> +   <li>The type must match the type loaded by the instruction.</li>
> +   <li>The pair <tt>a,b</tt> represents the unsigned range <tt>[a,b]</tt>.
> +   </li>
> +   <li>The range is allowed to wrap.</li>

these two lines are kind of mutually contradictory!  As you are using wrapping
ranges there is no difference between signed and unsigned ranges: they are both
handled, in a uniform way.  So I think you should remove the line about the
range being unsigned, and then just have enough examples (below) to make it
clear what you mean.

> +</ul>
> +
> +<p>Examples:</p>
> +<div class="doc_code">
> +<pre>
> +metadata !{ i8 0, i8 1 } ; Can only be 0 or 1

I think you should have the ranges map exactly to the ConstantRange class,
otherwise it is confusing to have more than one range concept inside the
compiler (and of course being able to use the ConstantRange methods directly
is convenient too).  As such ranges are open at the end, so { i8 0, i8 2 }
means "Can only be 0 or 1".

> +metadata !{ i8 0, i8 255 } ; Any i8

In such a case it would make more sense to not provide a range at all.  Wrapping
ranges always have the problem that there is an ambiguity about whether A .. A
represents the empty set or the full set.  The ConstantRange class has a way of
disambiguating this, but I think it would be better to just not give such
examples, which is OK since no-one is going to want to provide full or empty
ranges anyway.

> +metadata !{ i8 255, i8 1 } ; Can only be 255 (-1), 0 or 1

This would change to { i8 255, i8 2 }

The verifier changes look good to me.  I didn't look at the clang changes.

Ciao, Duncan.