[PATCH] D141471: [AArch64][SVE] Fix crash for DestructiveBinaryComm zero merging

[Allen zhong via Phabricator via llvm-commits]

Allen marked an inline comment as done.
Allen added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ExpandPseudoInsts.cpp:561
     // prefixed_zeroing_mov for DestructiveBinary.
-    assert((DOPRegIsUnique || AArch64::DestructiveBinary == DType) &&
+    assert((DOPRegIsUnique || (AArch64::DestructiveBinary == DType ||
+                               AArch64::DestructiveBinaryComm == DType)) &&
----------------
Allen wrote:
> paulwalker-arm wrote:
> > paulwalker-arm wrote:
> > > Allen wrote:
> > > > paulwalker-arm wrote:
> > > > > This being an || chain already in brackets I doubt you need these inner brackets.
> > > > Apply your comment, thanks
> > > I think you've misunderstood my comment and have also introduced a bug.  I meant there's no need for any new brackets i.e. you can write `(DOPRegIsUnique || AArch64::DestructiveBinary == DType || AArch64::DestructiveBinaryComm == DType) && ...`
> > > 
> > > Also with the new update you've incorrectly replaced the `DestructiveBinaryComm` comparison within an assignment of `DType`.
> > I've pushed a fix.
> Oh, sorry. Thanks very much
> Oh, sorry. Thanks very much




================
Comment at: llvm/lib/Target/AArch64/AArch64ExpandPseudoInsts.cpp:561
     // prefixed_zeroing_mov for DestructiveBinary.
-    assert((DOPRegIsUnique || AArch64::DestructiveBinary == DType) &&
+    assert((DOPRegIsUnique || (AArch64::DestructiveBinary == DType ||
+                               AArch64::DestructiveBinaryComm == DType)) &&
----------------
Allen wrote:
> Allen wrote:
> > paulwalker-arm wrote:
> > > paulwalker-arm wrote:
> > > > Allen wrote:
> > > > > paulwalker-arm wrote:
> > > > > > This being an || chain already in brackets I doubt you need these inner brackets.
> > > > > Apply your comment, thanks
> > > > I think you've misunderstood my comment and have also introduced a bug.  I meant there's no need for any new brackets i.e. you can write `(DOPRegIsUnique || AArch64::DestructiveBinary == DType || AArch64::DestructiveBinaryComm == DType) && ...`
> > > > 
> > > > Also with the new update you've incorrectly replaced the `DestructiveBinaryComm` comparison within an assignment of `DType`.
> > > I've pushed a fix.
> > Oh, sorry. Thanks very much
> > Oh, sorry. Thanks very much
> 
> 
> Oh, sorry. Thanks very much




================
Comment at: llvm/lib/Target/AArch64/AArch64ExpandPseudoInsts.cpp:561
     // prefixed_zeroing_mov for DestructiveBinary.
-    assert((DOPRegIsUnique || AArch64::DestructiveBinary == DType) &&
+    assert((DOPRegIsUnique || (AArch64::DestructiveBinary == DType ||
+                               AArch64::DestructiveBinaryComm == DType)) &&
----------------
Allen wrote:
> Allen wrote:
> > Allen wrote:
> > > paulwalker-arm wrote:
> > > > paulwalker-arm wrote:
> > > > > Allen wrote:
> > > > > > paulwalker-arm wrote:
> > > > > > > This being an || chain already in brackets I doubt you need these inner brackets.
> > > > > > Apply your comment, thanks
> > > > > I think you've misunderstood my comment and have also introduced a bug.  I meant there's no need for any new brackets i.e. you can write `(DOPRegIsUnique || AArch64::DestructiveBinary == DType || AArch64::DestructiveBinaryComm == DType) && ...`
> > > > > 
> > > > > Also with the new update you've incorrectly replaced the `DestructiveBinaryComm` comparison within an assignment of `DType`.
> > > > I've pushed a fix.
> > > Oh, sorry. Thanks very much
> > > Oh, sorry. Thanks very much
> > 
> > 
> > Oh, sorry. Thanks very much
> 
> 
I see your commit 78ba3e785cd


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141471