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

[Chad Rosier]

Lastly, do you think we could extend this work to handle something like
the following:

  while (i < e) {
    if (agg[i+1]->field < agg[i]->field)
      ++i;
    if (agg[i]->field > agg[j]->field)
      break;
    ...
    ...
  }

These two expressions seem equivalent:
agg[i+1]->field   // 1st if statement

and

++i
agg[i]->field     // 2nd if statement

However, I'm seeing a redundant load for agg[i]->field in the 2nd if
statement.

(Honestly, I feel like this patch is what would resolve this issue, but
I'm not 100%.)

Here is what I saw on AArch64 a while ago (~2 months):

	ldr	x16, [x8, w14, sxtw #3]   x16 = &agg[i]
	ldr	x17, [x8, w15, sxtw #3]   x17 = &agg[i+1]
	ldr	s0, [x16, #4]             s0 = agg[i]->field
	ldr	s1, [x17, #4]             s1 = agg[i+1]->field
	fcmp	s1, s0
	csel	w14, w15, w14, lt
	ldr	x16, [x8, w14, sxtw #3]   <==== load is redundant (and I believe your
patch would address this)
	ldr	s0, [x16, #4]             <==== load is redundant (but not this)
	ldr	s1, [x12, #4]             s1 = agg[j]->field
	fcmp	s0, s1
	b.gt	.LBB8_7                  // while exit

And I think we could generate something like this:

	fcmp	s1, s0
	csel	w14, w15, w14, lt
     	csel	x16, x17, x16, lt         <==== reuse value in x17/x16
        fcsel	s0, s1, s0, lt            <==== reuse value in s1/s0

Hopefully, this makes sense.  If not, I'm happy to clarify.

 Chad

> 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
>>>
>>
>>
>>
>
>
>