llvm dev - Apr 2017 - Is there any real downside to constructing the new SimplifyQuery once

For those not following along, startingin r301379, Simplify* in
InstructionSimplify now can just take a query struct  instead of 8000
optional arguments.  Nothing is really new since it used the same thing
under the covers.

I'm slowly converting the old uses away (deletion of the old APIs is a
different question).
Staring at most of them, i could just directly convert them using braced
list initialization, and construct the objects again and again, but the
vast majority of uses actually:

A. don't change the relevant query arguments over the lifetime of the pass
B. actually have more information sitting around than they are passing
along.

For example, instcombine has a DomTree and AssumptionCache that are
required, but it doesn't pass them to SimplifyBinOp in a bunch of cases.

JumpThreading has TLI but doesn't pass it to SimplifyCmpInst.

CVP at least admits it has a problem:
"CorrelatedValuePropagation.cpp:  // FIXME: Provide TLI, DT, AT to
SimplifyInstruction.
CorrelatedValuePropagation.cpp:  if (Value *V = SimplifyInstruction(P, DL))
{
"
(This is because it uses LVI, which requires those things, but it doesn't
ask for them itself)

Assuming this is not deliberate, it would seem to me to just be easiest to
set up the query structure once in the pass and use it everywhere, which
would hopefully avoid these issues in the future, besides making most of
the call strings dramatically shorter :)

Is there any real downside (compile time performance, etc) to this vs what
we do now (which is basically constructing the query objects again and
again under the covers) that anyone can think of?

(again, assuming we are not deliberately avoiding passing info to Simplify*
in most of these cases)

I figured i'd ask before i went writing the scripts/etc to help convert a
bunch of this stuff :)
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20170425/9e0d6e0e/attachment.html>

On 04/25/2017 11:32 PM, Daniel Berlin via llvm-dev
wrote:
> For those not following along, startingin r301379, Simplify* in 
> InstructionSimplify now can just take a query struct  instead of 8000 
> optional arguments.  Nothing is really new since it used the same 
> thing under the covers.
>
> I'm slowly converting the old uses away (deletion of the old APIs is a 
> different question).
> Staring at most of them, i could just directly convert them using 
> braced list initialization, and construct the objects again and again, 
> but the vast majority of uses actually:
>
> A. don't change the relevant query arguments over the lifetime of the
pass
> B. actually have more information sitting around than they are passing 
> along.
>
> For example, instcombine has a DomTree and AssumptionCache that are 
> required, but it doesn't pass them to SimplifyBinOp in a bunch of
cases.
>
> JumpThreading has TLI but doesn't pass it to SimplifyCmpInst.
>
> CVP at least admits it has a problem:
> "CorrelatedValuePropagation.cpp:  // FIXME: Provide TLI, DT, AT to 
> SimplifyInstruction.
> CorrelatedValuePropagation.cpp:  if (Value *V = SimplifyInstruction(P, 
> DL)) {
> "
> (This is because it uses LVI, which requires those things, but it 
> doesn't ask for them itself)
>
> Assuming this is not deliberate, it would seem to me to just be 
> easiest to set up the query structure once in the pass and use it 
> everywhere, which would hopefully avoid these issues in the future, 
> besides making most of the call strings dramatically shorter :)
>
> Is there any real downside (compile time performance, etc) to this vs 
> what we do now (which is basically constructing the query objects 
> again and again under the covers) that anyone can think of?
>
> (again, assuming we are not deliberately avoiding passing info to 
> Simplify* in most of these cases)
I don't think it is deliberate; it is just that no one has done the work 
to make it uniform. It is possible that doing this will expose some 
compile-time issues, but if so, we should deal with them for real (not 
just hide them by a failure to provide optional-but-available analyses).

I'm certainly in favor of more uniformity here.

  -Hal
>
> I figured i'd ask before i went writing the scripts/etc to help 
> convert a bunch of this stuff :)
>
>
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://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/20170426/e4ab5ff4/attachment.html>

Agreed.  This seems like overall a huge win for readability.

I am a bit worried about all the miscompiles you're going to expose by 
using all that extra information, but we should definitely just fix 
those.  :)

Philip


On 04/26/2017 08:53 AM, Hal Finkel via llvm-dev wrote:
>
>
> On 04/25/2017 11:32 PM, Daniel Berlin via llvm-dev wrote:
>> For those not following along, startingin r301379, Simplify* in 
>> InstructionSimplify now can just take a query struct  instead of 8000 
>> optional arguments.  Nothing is really new since it used the same 
>> thing under the covers.
>>
>> I'm slowly converting the old uses away (deletion of the old APIs
is
>> a different question).
>> Staring at most of them, i could just directly convert them using 
>> braced list initialization, and construct the objects again and 
>> again, but the vast majority of uses actually:
>>
>> A. don't change the relevant query arguments over the lifetime of
the
>> pass
>> B. actually have more information sitting around than they are 
>> passing along.
>>
>> For example, instcombine has a DomTree and AssumptionCache that are 
>> required, but it doesn't pass them to SimplifyBinOp in a bunch of
cases.
>>
>> JumpThreading has TLI but doesn't pass it to SimplifyCmpInst.
>>
>> CVP at least admits it has a problem:
>> "CorrelatedValuePropagation.cpp:  // FIXME: Provide TLI, DT, AT to
>> SimplifyInstruction.
>> CorrelatedValuePropagation.cpp:  if (Value *V = 
>> SimplifyInstruction(P, DL)) {
>> "
>> (This is because it uses LVI, which requires those things, but it 
>> doesn't ask for them itself)
>>
>> Assuming this is not deliberate, it would seem to me to just be 
>> easiest to set up the query structure once in the pass and use it 
>> everywhere, which would hopefully avoid these issues in the future, 
>> besides making most of the call strings dramatically shorter :)
>>
>> Is there any real downside (compile time performance, etc) to this vs 
>> what we do now (which is basically constructing the query objects 
>> again and again under the covers) that anyone can think of?
>>
>> (again, assuming we are not deliberately avoiding passing info to 
>> Simplify* in most of these cases)
>
> I don't think it is deliberate; it is just that no one has done the 
> work to make it uniform. It is possible that doing this will expose 
> some compile-time issues, but if so, we should deal with them for real 
> (not just hide them by a failure to provide optional-but-available 
> analyses).
>
> I'm certainly in favor of more uniformity here.
>
>  -Hal
>
>>
>> I figured i'd ask before i went writing the scripts/etc to help 
>> convert a bunch of this stuff :)
>>
>>
>>
>> _______________________________________________
>> LLVM Developers mailing list
>> llvm-dev at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
> -- 
> Hal Finkel
> Lead, Compiler Technology and Programming Languages
> Leadership Computing Facility
> Argonne National Laboratory
>
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://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/20170426/1e0cd992/attachment.html>