Commoning of target specific load/store intrinsics

[Pete Cooper]

Hi Sanjin

I’m not sure if there’s a better way to handle this, but i have some general comments on the style in the patch.

+  int  MatchingId; // Same Id is set for corresponding load/store intrinsics.

This should be an enum instead of an integer.  Same goes for all other definitions and uses of this variable.

+  /// \returns An instruction defining the result of a memory intrinsic.  New
+  /// instructions may be created to extract the result from an intrinsic memory
+  /// operation.
+  virtual Value* getOrCreateResultFromMemIntrinsic(IntrinsicInst *Inst,

You should mention that null is also a valid return value, if it was unable to get a valid intrinsic.

+bool AArch64TTI::getTgtMemIntrinsic
+    (IntrinsicInst* Inst, MemIntrinsicInfo& Info) const {

…

 +Value*
+TargetTransformInfo::getOrCreateResultFromMemIntrinsic(IntrinsicInst *Inst,
+                                                       Type *ExpectedType)
+  const {

Please run clang-format over the patch to see if there’s a better way to format long lines like these.

+Value* AArch64TTI::getOrCreateResultFromMemIntrinsic(IntrinsicInst* Inst,
+                                                     Type* ExpectedType) const {
+  Value* Res = nullptr;
+  switch (Inst->getIntrinsicID()) {
+  default:
+    break;
...
+  return Res;

I would just return nullptr from the default case here.  In fact i’d not have ‘Res’ at all in this method and just return from where you need to.

+  case Intrinsic::aarch64_neon_st4:
+    // Create a struct type
+    StructType *ST = dyn_cast<StructType>(ExpectedType);

You should have { } over the body of this case as you’ve defined a variable here which is going to potentially break if more cases add their own variables.

+  // Wrapper class to handle memory instructions, including loads, stores and
+  // intrinsic loads and stores defined by the target.
+  class ParseMemoryInst {

/// and \brief to have the style of your other code.

+          if (TTI->getTgtMemIntrinsic(II, Info)) {
+            if (Info.NumMemRefs == 1) {
+              Store = Info.WriteMem;
+              Load = Info.ReadMem;
+              MayReadFromMemory = Info.ReadMem;
+              MayWriteToMemory = Info.WriteMem;
+              Vol = Info.Vol;
+              Ptr = Info.PtrVal;
+            }
+          }

You can reduce nesting with 'if (!TTI->getTgtMemIntrinsic(II, Info)) return'

+    ParseMemoryInst MemInst(Inst, TTI);
     // If this is a non-volatile load, process it.
-    if (LoadInst *LI = dyn_cast<LoadInst>(Inst)) {
+    if (MemInst.isLoad()) {

I think here you need to first check if MemInst was constructed before you can safely check isLoad.  You may want to add an isValid method and change it to ‘ MemInst.isValid() && MemInst.isLoad()’.  Probably similarly for later uses.  Alternatively you could override bool operator() instead of defining isValid().

-    if (Inst->mayReadFromMemory())
+    if (MemInst.mayReadFromMemory())

This isn’t a valid change as the implementation of Inst->mayReadFromMemory() handles things like Instruction::VAArg which ParseMemoryInst doesn’t.

Thanks,
Pete
> On Jan 16, 2015, at 12:28 PM, Sanjin Sijaric <ssijaric at codeaurora.org> wrote:
> 
> 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?
>  
> Thanks,
> Sanjin
> <CSEPATCH>_______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu <mailto:llvm-commits at cs.uiuc.edu>
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits <http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150116/8ce3a7f9/attachment.html>