Commoning of target specific load/store intrinsics

[Hal Finkel]

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