[PATCH] D96677: [AVR] Expand large shifts early in IR

[Ben Shi via Phabricator via llvm-commits]

benshi001 added inline comments.


================
Comment at: llvm/lib/Target/AVR/AVRShiftExpand.cpp:52
+  auto &Ctx = F.getContext();
+  for (inst_iterator II = inst_begin(F), E = inst_end(F); II != E; ++II) {
+    Instruction *I = &*II;
----------------
can this line be simpilfied to 

```
for (Instruction &I : instructions(F))
```

as in `Transforms/IPO/Attributor.h` ?


================
Comment at: llvm/lib/Target/AVR/AVRShiftExpand.cpp:54-63
+    if (!I->isShift())
+      // Only expand shift instructions (shl, lshr, ashr).
+      continue;
+    if (I->getType() != Type::getInt32Ty(Ctx))
+      // Only expand plain i32 types.
+      continue;
+    if (isa<ConstantInt>(I->getOperand(1)))
----------------
Is it better to combine all these conditions to only one if statement?


================
Comment at: llvm/lib/Target/AVR/AVRShiftExpand.cpp:67-70
+  if (ShiftInsts.size() == 0)
+    // There are no shift instructions in this function that should be expanded
+    // to loops.
+    return false;
----------------
Is this line needed?

the following for-statement won't run if `ShiftInsts.size() == 0`

And we just return `ShiftInsts.size() > 0` at the end.


================
Comment at: llvm/lib/Target/AVR/AVRShiftExpand.cpp:92
+  // AVR register.
+  Value *ShiftAmount = Builder.CreateTrunc(BI->getOperand(1), Int8Ty);
+
----------------
Is it possible to generate a check about `ShiftAmount>32`, like that

```
  cmp ShiftAmount, 32
  brlt _labloop
  mov Rdst, 0
  ret
_labloop:
  the shift loop
```

However avr-gcc does not generate the check against 32.


================
Comment at: llvm/test/CodeGen/AVR/shift-expand.ll:11
+; CHECK-LABEL: @shl
+; CHECK:   %3 = trunc i32 %1 to i8
+; CHECK:   %4 = icmp eq i8 %3, 0
----------------
How about try to generate instruction serial with llvm/utils/update_llc_test_checks.py ? It already support AVR.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96677