[PATCH] D57071: GlobalISel: Handle more cases for widenScalar for G_STORE

[Amara Emerson via Phabricator via llvm-commits]

aemerson added inline comments.


================
Comment at: lib/CodeGen/GlobalISel/LegalizerHelper.cpp:934
   case TargetOpcode::G_STORE: {
-    if (MRI.getType(MI.getOperand(0).getReg()) != LLT::scalar(1) ||
-        WideTy != LLT::scalar(8))
+    if (TypeIdx != 0)
+      return UnableToLegalize;
----------------
arsenm wrote:
> aemerson wrote:
> > Why is this being changed?
> This is the entire problem. This was only handling the 1-to-8 bit case. All the AMDGPU 8 or 16-bit stores use 32-bit source registers
Oh sorry, for some reason I thought this hunk was the before-diff not the *after*.


================
Comment at: lib/CodeGen/GlobalISel/LegalizerHelper.cpp:946
+    unsigned ExtType = Ty.getScalarSizeInBits() == 1 ?
+      TargetOpcode::G_ZEXT : TargetOpcode::G_ANYEXT;
+    widenScalarSrc(MI, WideTy, 0, ExtType);
----------------
arsenm wrote:
> aemerson wrote:
> > SelectionDAG does zext for the same IR for all types, so I think we should be consistent there.
> I don't think that's true? 
> 
> In this test case for example, in PromoteIntOp_STORE/PromoteIntRes_LOAD it decided to use the morally equivalent ISD::EXTLOAD extload type.
> 
> define void @foo(i8 addrspace(1)* %in, i8 addrspace(1)* %out) {
>   %val = load i8, i8 addrspace(1)* %in
>   store i8 %val, i8 addrspace(1)* %out
>   ret void
> }
> 
> The high bits also won't matter for a truncating store
So I was using a test case like this to check:

```
define void @test(i12 %v, i12 *%ptr) {
  store i12 %v, i12* %ptr
  ret void
}
```

I get your point that in theory it should be semantically correct to do anyext. However I'd rather not we change the way i1s are handled.


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

https://reviews.llvm.org/D57071