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

[Tilmann Scheller]

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
>