[PATCH] D62604: [CodeGen] Generic Hardware Loop Support

[Sam Parker via Phabricator via llvm-commits]

samparker marked 5 inline comments as done.
samparker added inline comments.


================
Comment at: include/llvm/Analysis/TargetTransformInfo.h:462
+    IntegerType *CountType    = nullptr;
+    Value *NumElements        = nullptr;  // The maximum number of elements
+                                          // processed in the loop body.
----------------
markus wrote:
> Is `NumElements` really required by the initial commit that only adds the loop intrinsics?
Not strictly, but it makes the insertion of the loop.decrement intrinsic cleaner. In this patch, this value is always '1' and used several times, so it's nice to have this initialised and stored for re-use.


================
Comment at: include/llvm/Analysis/TargetTransformInfo.h:468
+                                          // in the loop via a phi?
+    bool RequiresNewPreheader = false;
+  };
----------------
markus wrote:
> I find this confusing. I understand that a preheader is required for the transformation (because that is where the `llvm.set.loop.iterations` is inserted) but why would an additional/new preheader ever be needed?
The current use case is when the existing preheader could interfere with whatever method the architecture uses for the loops. For PowerPC they don't want the mctr register being incorrectly written.


================
Comment at: include/llvm/IR/Intrinsics.td:1185
 
+def int_set_loop_iterations :
+  Intrinsic<[], [llvm_anyint_ty], [IntrNoDuplicate]>;
----------------
SjoerdMeijer wrote:
> Do we need to document these intrinsics? Do they need to be 'experimental' first, like the ones above?
I'm honestly unsure when *experimental* is needed. I've assumed they're normally used when the operation maybe slightly ambiguous or needs to be interpreted by a lot of the rest of the infrastructure.


================
Comment at: lib/CodeGen/HardwareLoops.cpp:180
+    if (!L->getParentLoop())
+      TryConvertLoop(L);
+  }
----------------
SjoerdMeijer wrote:
> Do we want to check the return value?
It's not neeeded, TryConvertLoop returns a value to control its recursive search. MadeChange is a member and is written by it.


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:9840
 
-    assert(N->getValueType(0) == MVT::i1 &&
-           "Unexpected result type for CTR decrement intrinsic");
+    //assert(N->getValueType(0) == MVT::i1 &&
+      //     "Unexpected result type for CTR decrement intrinsic");
----------------
SjoerdMeijer wrote:
> Should this be commented out? Remove this?
Ah, cheers. Will need to take a look at that..


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62604/new/

https://reviews.llvm.org/D62604