[PATCH] D85956: [AARCH64][RegisterCoalescer] clang miscompiles zero-extension to long long

[Simon Wallis via Phabricator via llvm-commits]

simonwallis2 added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64RegisterInfo.cpp:735
+    const TargetRegisterClass *NewRC, LiveIntervals &LIS) const {
+  if (MI->isCopy() && (DstRC->getID() == AArch64::GPR64RegClassID) &&
+      (SrcRC == DstRC) && MI->getOperand(0).getSubReg())
----------------
john.brawn wrote:
> We should have a comment here explaining why coalescing is incorrect in this case. Also I'm not sure if the SrcRC == DstRC is correct here, as the implicit zeroing on subregister write doesn't depend on what the source register is.
> We should have a comment here explaining why coalescing is incorrect in this case. 
I will reword and tailor the comment that is in the test case.




================
Comment at: llvm/lib/Target/AArch64/AArch64RegisterInfo.cpp:735
+    const TargetRegisterClass *NewRC, LiveIntervals &LIS) const {
+  if (MI->isCopy() && (DstRC->getID() == AArch64::GPR64RegClassID) &&
+      (SrcRC == DstRC) && MI->getOperand(0).getSubReg())
----------------
simonwallis2 wrote:
> john.brawn wrote:
> > We should have a comment here explaining why coalescing is incorrect in this case. Also I'm not sure if the SrcRC == DstRC is correct here, as the implicit zeroing on subregister write doesn't depend on what the source register is.
> > We should have a comment here explaining why coalescing is incorrect in this case. 
> I will reword and tailor the comment that is in the test case.
> 
> 
> Also I'm not sure if the SrcRC == DstRC is correct here, as the implicit zeroing on subregister write doesn't depend on what the source register is.

Thanks. I will modify the check. Currently it matches:
```
undef %4.sub_32:gpr64 = COPY %2.sub_32:gpr64
```
The 2 register classes do not have to be the same, 
But they both have to be a subregister of a 64bit int register type so that this will also match:
```
undef %40.sub_32:gpr64 = COPY %41.sub_32:gpr64common
```
So I should include GPR64common.


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

https://reviews.llvm.org/D85956