[PATCH] Fix GlobalOpt to use range metadata instead of creating SelectInsts.
rafael.espindola at gmail.com
Wed Mar 12 09:56:38 PDT 2014
> If the comparison doesn't fold and it remains a ConstantExpr, this code
> still works. A ConstantExpr isn't as nice of a range bound as a ConstantInt,
> but if that's all we have, that's all the select would have. If we converted
> it to use integer values directly, or APInts rather, it wouldn't handle that
> case. Also, the inputs and outputs are already both Constants, and there's
> not that much code here.
The verifier asserts that the ranges are composed of ConstantInts.
That is why I find using APInt and building limits directly better. It
makes it obvious that we are producing ranges with the right type.
>> The new optimization is a bit simpler and a bit less likely to upset
>> other optimizations, but it is also a bit less powerful. Can you
>> explain why you want the change? Have you hit a case where the select
>> was detrimental?
> Yes, I have a benchmark where the select disrupts other optimizations and is
> detrimental. Also, it seems intuitively bad to speculatively create selects
> hoping that it'll help subsequent optimizations more than it hurts.
> This patch is still powerful enough to optimize the bzip2 benchmark that
> originally motivated this optimization. It's only less powerful in that it
> indicates a range of values instead of just the two discrete values, but it
> seems most existing optimizations just care about the range. If more precise
> information is desired, it would make sense to augment the range metadata to
> be able to provide that. If that is done, then this optimization could
> easily be augmented to support it.
The range should already support that if need. Only the optimization
would need to up updated to created multiple ranges.
One more nit, can you update test/Transforms/GlobalOpt/integer-bool.ll
to also CHECK the range added to the load?
The patch LGTM since the verifier takes care of making sure we have
ConstantInts, but I would still suggest using APInts directly.
More information about the llvm-commits