[PATCH] D137108: Implement support for AArch64ISD::MOVI in computeKnownBits

[Dave Green via Phabricator via llvm-commits]

dmgreen added a comment.

There are other MOVI nodes created in ISel, have you considered adding support for those too?

  MOVI,
  MOVIshift,
  MOVIedit,
  MOVImsl,
  FMOV,
  MVNIshift,
  MVNImsl,



================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:1976
+  case AArch64ISD::MOVI: {
+    ConstantSDNode *CN = cast<ConstantSDNode>(Op->getOperand(0));
+    assert(CN && "Expect MOVI operand to be ConstantSDNode");
----------------
foad wrote:
> tschuett wrote:
> > tschuett wrote:
> > > Could you you change it to `dyn_cast` with if to get the correct assert and error message?
> > I still believe that you need and `if` to check in Release mode whether it really is a constant node.
> No, just use cast<> and remove the assert.
cast will do the assert for you. I would think it would be OK to always use case and drop the assert.

The operand of a MOVI will always be a ConstantSDNode. There is no need to check for it in release builds, it should be correct by construction.


================
Comment at: llvm/test/CodeGen/AArch64/shift-accumulate.ll:124
+
+; Expected to be able to deduce movi is generate a vector of integer
+; and turn USHR+ORR into USRA.
----------------
adriantong1024 wrote:
> foad wrote:
> > adriantong1024 wrote:
> > > foad wrote:
> > > > Pre-commit this test and rebase the current patch on top of it?
> > > Hi Jay
> > > 
> > > Thank you for the comment. Do you mean I should change this test and commit it as a separate patch before the actual change (i.e. the implementation of the MOVI) ?
> > Yes. Commit the test in a form that will pass on trunk, and then when you rebase this patch, it'll show the difference in the codegen caused by your patch. That makes it much easier for reviewers to see the effect of your patch.
> > 
> > (I'm not an AArch64 reviewer but this is pretty common practice for the backends I've worked on.)
> Makes sense !. I will do so once this patch is accepted. Thanks for teaching me this.
Can you expand this to other types too. For example this, as well as i64 variants:
```
define <4 x i32> @usra_with_movi16(<8 x i16> %0, <8 x i16> %1) {
; CHECK-LABEL: usra_with_movi:
; CHECK:       // %bb.0:
; CHECK-NEXT:    movi v2.16b, #1
; CHECK-NEXT:    cmeq v0.16b, v0.16b, v1.16b
; CHECK-NEXT:    and v0.16b, v0.16b, v2.16b
; CHECK-NEXT:    usra v0.8h, v0.8h, #7
; CHECK-NEXT:    ret
  %3 = icmp eq <8 x i16> %0, %1
  %4 = zext <8 x i1> %3 to <8 x i16>
  %5 = bitcast <8 x i16> %4 to <4 x i32>
  %6 = lshr <4 x i32> %5, <i32 7, i32 7, i32 7, i32 7>
  %7 = or <4 x i32> %6, %5
  ret <4 x i32> %7
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137108