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

[Ayke via Phabricator via llvm-commits]

aykevl 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;
----------------
benshi001 wrote:
> can this line be simpilfied to 
> 
> ```
> for (Instruction &I : instructions(F))
> ```
> 
> as in `Transforms/IPO/Attributor.h` ?
Thank you! Yes, that looks a lot better.


================
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)))
----------------
benshi001 wrote:
> Is it better to combine all these conditions to only one if statement?
I think keeping them separate is better for readability. Now every condition has a comment explaining why the condition needs to be checked.


================
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;
----------------
benshi001 wrote:
> 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.
I've modified the code accordingly. Either seems fine by me.


================
Comment at: llvm/lib/Target/AVR/AVRShiftExpand.cpp:92
+  // AVR register.
+  Value *ShiftAmount = Builder.CreateTrunc(BI->getOperand(1), Int8Ty);
+
----------------
benshi001 wrote:
> 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.
Shifts larger or equal to the bit width (>=32) are a poison value according to the LangRef. Therefore, in practice they should not occur. I believe they're undefined behavior according to the C standard.


================
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
----------------
benshi001 wrote:
> How about try to generate instruction serial with llvm/utils/update_llc_test_checks.py ? It already support AVR.
Good idea, I'll use update_test_checks.py.

Note that the output is IR, not AVR assembly. This is a pure IR level pass.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96677