[PATCH] D132325: [AArch64][CodeGen] Fold the mov and lsl into ubfiz

[Allen zhong via Phabricator via llvm-commits]

Allen added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.td:7348
 def : Pat<(shl (i64 (sext GPR32:$Rn)), (i64 imm0_63:$imm)),
-          (SBFMXri (INSERT_SUBREG (i64 (IMPLICIT_DEF)), GPR32:$Rn, sub_32),
+          (SBFMXri (SUBREG_TO_REG (i64 0), GPR32:$Rn, sub_32),
+                   (i64 (i64shift_a        imm0_63:$imm)),
----------------
efriedma wrote:
> paulwalker-arm wrote:
> > @efriedma / @Allen : Sorry for the naive question but I'm not hugely familiar with this node and the documentation says:
> > ```
> > /// All other bits are
> > /// assumed to be equal to the bits in the immediate integer constant in the
> > /// first operand. This instruction just communicates information; No code
> > /// should be generated.
> > ```
> > So is this safe? I mean if no code is generated then in this instance how can we be sure `$Rn` has it's top 32bits zeroed? I'm kind of assuming this is why the original code is using `INSERT_SUBREG`?
> > 
> > The emitted code is valid, but could something query the `SUBREG_TO_REG` post isel and reuse/transform it in an invalid way?
> Oh, I didn't notice it was changed from using INSERT_SUBREG to use SUBREG_TO_REG.  I just glanced over the patterns and assumed it was just using the same pattern we were already using.  Not sure why it was changed.  We normally only use SUBREG_TO_REG if we know the operand actually clears the high bits.
> 
> Not sure there's really any practical consequence here; there isn't very much code that's actually aware of the semantics of SUBREG_TO_REG.
hi @paulwalker-arm @efriedma
    According https://developer.arm.com/documentation/ddi0602/2021-12/Base-Instructions/UBFIZ--Unsigned-Bitfield-Insert-in-Zero--an-alias-of-UBFM-?lang=en, the insn ubfiz  copies specified bits from the source $Rn and clears other unspecified bits. 
   In this pattern, the top 32bits will be clean, so I think it is save.

If we use the original **INSERT_SUBREG**, I find the test case **@loop2** in file llvm/test/CodeGen/AArch64/tbl-loops.ll regressses.
```
+++ b/llvm/test/CodeGen/AArch64/tbl-loops.ll
@@ -151,7 +151,8 @@ define void @loop2(i8* noalias nocapture noundef writeonly %dst, float* nocaptur
 ; CHECK-NEXT:    cmp w8, #2
 ; CHECK-NEXT:    b.ls .LBB1_4
 ; CHECK-NEXT:  // %bb.2: // %vector.memcheck
-; CHECK-NEXT:    ubfiz x9, x8, #1, #32
+; CHECK-NEXT:    mov w9, w8
+; CHECK-NEXT:    ubfiz x9, x9, #1, #32
```
I think this is because the extra **%37: grp64all = IMPLICIT_DEF** need alloc a register for %37, which is not always specified the same register of %36
**%36:gpr64 = INSERT_SUBREG %37：grp64all (tied-def 0), %24:gpr32common, %subreg.sub_32**


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

https://reviews.llvm.org/D132325