[PATCH] [InstCombine] Attempt to eliminate redundant loads whose addresses are dependent on the result of a select instruction.

[Chad Rosier]

Tilmann,
Would you consider filing a bug with an outline of the proposed solution? 
I'd hate for this work to stall in your low priority queue for too long;
happens to me all the time.  Perhaps, someone else might consider picking
up the work?

Also, is there a specific benchmark you're targeting?  We're doing a lot
of AArch64 tuning on my end and I'm trying to track which changes are
effecting which benchmarks.  If there's a big enough win, I might be able
to redirect my work and address this on your behalf (assuming you don't
mind).  Please let me know.

  Chad

> Hi Chad,
>
> thanks for the review!
>
> I discussed this earlier this week with David on IRC and he pointed me to
> the PHITransAddr class, suggesting to implement something similar for
> selects. That way GVN doesn't need to know much about selects and
> everything
> will just go through MemDep.
>
> I think this is a much better solution but I haven't actually started
> working on it (and most likely won't be able to for the next couple of
> weeks).
>
> Regards,
>
> Tilmann
>
> -----Original Message-----
> From: Chad Rosier [mailto:mcrosier at codeaurora.org]
> Sent: Thursday, September 18, 2014 4:46 AM
> To: Tilmann Scheller
> Cc: 'David Majnemer'; llvm-commits at cs.uiuc.edu
> Subject: RE: [PATCH] [InstCombine] Attempt to eliminate redundant loads
> whose addresses are dependent on the result of a select instruction.
>
> Tilmann,
> GVN is certainly not my area, but hopefully my comments will be helpful.
> Overall, the patch LGTM, but perhaps David or others have additional
> comments (i.e., I'm not the right person to approve this patch :).
>
> +// Helper function for OptimizeLoadGEPSelect().
> +static bool IsMatchingLoadGEP(Value *Inst, GetElementPtrInst *BaseGEP,
> +                              Value *&SecondIdx) {
>
> Hoist isa load logic out and add an assert(isa<LoadInst>(Inst) &&
> "Expected
> Load");
>
> +      bool FoundTrue = false;
> +      bool FoundFalse = false;
>
> These two booleans, FoundTrue and FoundFalse, seem redundant with LoadTrue
> and LoadFalse, respectively.
>
> +      Value *LoadTrue = nullptr;
> +      Value *LoadFalse = nullptr;
>
> +      while (ScanFrom != ScanBB->begin() &&
> +             !(FoundTrue && FoundFalse)) {
>
> How about reusing LoadTrue and LoadFalse?
>
>   !(LoadTrue && LoadFalse)
>
> +        Instruction *Inst = --ScanFrom;
> +
> +        // Just give up if memory is potentially modified.
> +        if (Inst->mayWriteToMemory()) {
> +          return nullptr;
> +        }
>
> Curly braces not necessary:
>
>        if (Inst->mayWriteToMemory())
>         return nullptr;
>
>
> +        Value *SecondIdx = nullptr;
> +        if (IsMatchingLoadGEP(Inst, GEPI, SecondIdx)) {
>
> Hoist the isa load check out of the help function?
>
>   if (isa<LoadInst>(Inst) && IsMatchingLoadGEP(Inst, GEPI, SecondIdx)
>
> +      // All preconditions fulfilled, apply the transformation:
> +      if (FoundTrue && FoundFalse)
>
> How about?
>
>   if (LoadTrue && LoadFalse)
>
> I would like to see this go in as I've seen this issue in the past.
>
>  Chad
>
>> Hi David,
>>
>> attached is a new patch which is now doing the transformation in GVN.
>>
>> The bulk of the patch is still the same, I didn't find much logic
>> within GVN which I could reuse, if there's any obvious way to simplify
>> the patch please let me know :)
>>
>> Nonetheless, the optimization is now in close distance to the other
>> redundant load optimizations.
>>
>> Regards,
>>
>> Tilmann
>>
>> -----Original Message-----
>> From: Tilmann Scheller [mailto:t.scheller at samsung.com]
>> Sent: Thursday, September 04, 2014 8:23 AM
>> To: 'David Majnemer'
>> Cc: 'llvm-commits at cs.uiuc.edu'
>> Subject: RE: [PATCH] [InstCombine] Attempt to eliminate redundant
>> loads whose addresses are dependent on the result of a select
>> instruction.
>>
>> Hi David,
>>
>> fair enough, I'll try to figure out how to implement this in GVN then
>> :)
>>
>> Regards,
>>
>> Tilmann
>>
>> From: David Majnemer [mailto:david.majnemer at gmail.com]
>> Sent: Thursday, September 04, 2014 12:15 AM
>> To: Tilmann Scheller
>> Cc: llvm-commits at cs.uiuc.edu
>> Subject: Re: [PATCH] [InstCombine] Attempt to eliminate redundant
>> loads whose addresses are dependent on the result of a select
>> instruction.
>>
>> Hi Tilmann,
>>
>> I don't think this a natural place to do this, I'm of the opinion that
>> we should try to get GVN to do this sort of thing.
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>
>
>
>