[PATCH] D50623: [NFC] [Hexagon] Simplify int8 truncation and validation

[Kim Gräsman via Phabricator via llvm-commits]

kimgr added inline comments.


================
Comment at: lib/Target/Hexagon/HexagonBitSimplify.cpp:1980
-    case Hexagon::S2_storerb_io:
-      V = int8_t(U);
-      break;
----------------
kparzysz wrote:
> kimgr wrote:
> > kparzysz wrote:
> > > These casts are actually necessary.  Those are store-register instructions that take a 32-bit register and store the lower 8, 16 or 32 bits, and this code tries to replace them with the equivalent store-immediate instructions.  For example, if the register contains 0x001000FF, the S2_storerb_io instruction will store 0xFF and can be replaced with a store-immediate of -1.
> > Oh, now I see what's going on... The truncation by int cast is a little subtle. Maybe transform `U` instead, something like:
> > 
> > ```
> > switch (Opc) {
> >   case Hexagon::S2_storerb_io:
> >     U &= UINT8_MAX;
> >     break;
> >   case Hexagon::S2_storerh_io:
> >     U &= UINT16_MAX;
> >     break;
> >   case Hexagon::S2_storeri_io:
> >     U &= UINT32_MAX;
> >     break;
> > };
> > 
> > int64_t V = U;
> > if (!isInt<8>(V))
> >   return false;
> > ```
> > 
> > ?
> > 
> That performs zero-extension, what we want is sign-extension: 0x000000FF needs to become 0xFFFFFFFF.
Thanks, I see that now. I'm a little hesitant to suggest anything else, because I clearly have no idea what I'm doing :)

It turns out we can convince GCC using `llvm_unreachable` -- I think that's a nice surgical change:

```
  // Only consider 8-bit values to avoid constant-extenders.
  int V;
  switch (Opc) {
    case Hexagon::S2_storerb_io:
      V = int8_t(U);
      break;
    case Hexagon::S2_storerh_io:
      V = int16_t(U);
      break;
    case Hexagon::S2_storeri_io:
      V = int32_t(U);
      break;
    default:
      llvm_unreachable("Unexpected store opcode");
  }
  if (!isInt<8>(V))
    return false;
```

That makes more sense, right?


Repository:
  rL LLVM

https://reviews.llvm.org/D50623