Commoning of target specific load/store intrinsics

[Sanjin Sijaric]

Hi Hal,

Thanks for looking at this.

The check for InVal.second == CurrentGeneration is still there.   The call to getOrCreateResult occurs after the check passes, and returns nullptr in cases where there is a mismatch between the intrinsic store and a load.  For example,

call void @llvm.aarch64.neon.st2.v4i32.p0i8(<4 x i32> %3, <4 x i32> %4, i8* %0) <==== This will be InVal.first when we encounter the load below.
%vld3 = call { <4 x i32>, <4 x i32>, <4 x i32> } @llvm.aarch64.neon.ld3.v4i32.p0i8(i8* %0) <==== The expected type is { <4 x i32>, <4xi32>, <4 x i32> }, but the type that can be extracted from st2 is { <4 x i32>, <4 x i32> }.  No commoning can occur.  

I'll send the updated patch with more test cases.

Thanks,
Sanjin

-----Original Message-----
From: Hal Finkel [mailto:hfinkel at anl.gov] 
Sent: Saturday, January 17, 2015 7:35 AM
To: Sanjin Sijaric
Cc: llvm-commits at cs.uiuc.edu
Subject: Re: Commoning of target specific load/store intrinsics

----- Original Message -----
> From: "Sanjin Sijaric" <ssijaric at codeaurora.org>
> To: llvm-commits at cs.uiuc.edu, "Hal Finkel" <hfinkel at anl.gov>
> Sent: Friday, January 16, 2015 2:28:17 PM
> Subject: Commoning of target specific load/store intrinsics
> 
> Hi,
> 
> We have a need to common target specific load/store intrinsics (in our 
> case, AArch64 intrinsics). This patch handles commoning of a few of 
> these intrinsics in Early CSE. We modified the Early CSE a bit to get 
> the necessary information from the target. Is there a more appropriate 
> place to handle this other than Early CSE, or a better way to handle 
> it in Early CSE?
> 
> 

I think generally this looks good. A couple of comments:


-        AvailableLoads->lookup(Inst->getOperand(0));
+        AvailableLoads->lookup(MemInst.getPtr());
       if (InVal.first != nullptr && InVal.second == CurrentGeneration) {
-        DEBUG(dbgs() << "EarlyCSE CSE LOAD: " << *Inst << "  to: "
-              << *InVal.first << '\n');
-        if (!Inst->use_empty()) Inst->replaceAllUsesWith(InVal.first);
-        Inst->eraseFromParent();
-        Changed = true;
-        ++NumCSELoad;
-        continue;
+        Value *Op = getOrCreateResult(InVal.first, Inst->getType());
+        if (Op != nullptr) {
+          DEBUG(dbgs() << "EarlyCSE CSE LOAD: " << *Inst << "  to: "
+                << *Op << '\n');

What happened to the InVal.second == CurrentGeneration check?

+++ b/test/Transforms/EarlyCSE/AArch64/intrinsics.ll

...

You definitely need more test cases here. You've only tested one of the pairings, and you don't have any tests where intervening stores prevent the combination.

 -Hal

> 
> Thanks,
> 
> Sanjin

--
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory