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

[Hal Finkel via Phabricator via llvm-commits]

hfinkel added a comment.

In D62604#1528869 <https://reviews.llvm.org/D62604#1528869>, @markus wrote:

> What happens if a target decides late (like `addPreEmitPass()` late) that a hardware loop is no longer possible due to reasons that are not detectable from the IR level hook. For PowerPC this does not seem to be an issue,


For PowerPC, it is an issue. We deal with this by checking, at the IR level, for all constructs that could cause the later steps to fail (or be invalid). We even have a dedicated MI-level verification pass which will assert if we missed something in this regard. This works, but is fragile. I like the idea discussed here of having a fallback lowering (although this patch seems to preserve the current scheme).

> or at least I cannot find it in the code, but for other targets there could be limitations that you need to analyse the actual machine instructions to find out about. Since the original IV chain has been replaced by intrinsic `llvm.loop.decrement.reg` (or whatever it iselected to) it seems we now would need to manually insert instructions to update the loop counter which could possibly be a bit hairy to do this late.
> 
> On the other hand (but not saying that it is better) if one treats the intrinsic as a pure hint and keep the original loop counter intact then the problem here becomes easier as it would just be a matter of deleting instructions if hardware loop insertion is successful and doing nothing if it fails.





================
Comment at: lib/CodeGen/HardwareLoops.cpp:326
+  ExitCount = SE.getAddExpr(ExitCount, SE.getOne(CountType));
+  Value *Count = SCEVE.expandCodeFor(ExitCount, CountType,
+                                     BB->getTerminator());
----------------
Are you sure that isSafeToExpand[At] will always be true here? I recommend checking explicitly and then dealing in the callers of this function with the fact that it might fail.


================
Comment at: lib/Target/PowerPC/PPCTargetTransformInfo.h:56
   TTI::PopcntSupportKind getPopcntSupport(unsigned TyWidth);
+  bool mightUseCTR(BasicBlock *BB, TargetLibraryInfo *LibInfo);
+  bool isHardwareLoopProfitable(Loop *L, ScalarEvolution &SE,
----------------
This can't have this name here, as CTR refers to a PowerPC-specific register. Shouldn't this just be folded into the isHardwareLoopProfitable implementation?


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

https://reviews.llvm.org/D62604