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

[Chad Rosier]

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
>