[PATCH] D111237: [TypePromotion] Promote PHI + [SZ]Ext

[Dave Green via Phabricator via llvm-commits]

dmgreen added a reviewer: samparker.
dmgreen added a comment.

Here's a round of review. I haven't got though everything in TryToPromotePHI yet, but it mostly seemed sensible from what I could tell. Reducing the indenting would be good, if we can.



================
Comment at: llvm/lib/CodeGen/TypePromotion.cpp:955
+  EVT ExtVT = TLI->getValueType(DL, ExtI->getType());
+  if (!ExtVT.isVector() && RegisterBitWidth < ExtVT.getFixedSizeInBits()) {
+    LLVM_DEBUG(dbgs() << "IR Promotion: extension type does not fit into "
----------------
It may be possible to check for legal types, as opposed to using this single RegisterBitWidth (and ignoring vector sizes).


================
Comment at: llvm/lib/CodeGen/TypePromotion.cpp:981
+          if (Id != Intrinsic::masked_load) {
+            Transform = false;
+            break;
----------------
I think you may be able to turn any `Transform = false; break;` into `return false;`


================
Comment at: llvm/lib/CodeGen/TypePromotion.cpp:990
+        // Check whether the target can merge the Load and Extend instructions.
+        InstructionCost Cost = TTI.getCastInstrCost(
+            ExtI->getOpcode(), ExtI->getType(), LoadI->getType(),
----------------
It may be better to check for legal operations instead of the cost. Not sure though with the masked operations.


================
Comment at: llvm/lib/CodeGen/TypePromotion.cpp:1007
+              continue;
+            if (isa<Instruction>(U.getUser())) {
+              auto *UseI = cast<Instruction>(U.getUser());
----------------
The user of an instruction will always be an instruction. llvm likes to use continue to reduce heavy indenting too, where it can.


================
Comment at: llvm/lib/CodeGen/TypePromotion.cpp:1075
+      } else if (isa<PHINode>(IncValue) && IncValue->hasOneUse())
+        Worklist.push_back(cast<PHINode>(IncValue));
+    }
----------------
Is this reachable?


================
Comment at: llvm/lib/CodeGen/TypePromotion.cpp:1174
 
-  // Search up from icmps to try to promote their operands.
+  SmallVector<Instruction *, 20> ToRemove;
+  SmallPtrSet<PHINode *, 20> VisitedPHIs;
----------------
20 is quite high, it is usually around 4. I think you can leave the amount off in most cases and it will choose a sensible default.


================
Comment at: llvm/lib/CodeGen/TypePromotion.cpp:1178
     for (auto &I : BB) {
-      if (AllVisited.count(&I))
-        continue;
+      if (isa<Instruction>(&I) && (I.getOpcode() == Instruction::ZExt ||
+                                   I.getOpcode() == Instruction::SExt)) {
----------------
I will always be an Instruction.


================
Comment at: llvm/lib/CodeGen/TypePromotion.cpp:1199
 
-      LLVM_DEBUG(dbgs() << "IR Promotion: Searching from: " << *ICmp << "\n");
+      if (isa<ICmpInst>(&I)) {
+        auto *ICmp = cast<ICmpInst>(&I);
----------------
It is preferred in llvm to remove indenting with early exits/continues. From what I can tell, this algorithm is in two stages now, and this code needn't change? The phis are optimized first, then sequences starting from icmps.


================
Comment at: llvm/test/Transforms/TypePromotion/AArch64/dont-promote-phi-ext.ll:5
+; Function Attrs: nounwind uwtable
+define dso_local i32 @f(i8* nocapture readonly %ip) local_unnamed_addr {
+; CHECK-LABEL: @f(
----------------
I like to remove "; Function Attrs:..", "dso_local" and "local_unnamed_addr"


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

https://reviews.llvm.org/D111237