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

[Krzysztof Parzyszek via Phabricator via llvm-commits]

kparzysz added inline comments.


================
Comment at: lib/Target/Hexagon/HexagonBitSimplify.cpp:1980
-    case Hexagon::S2_storerb_io:
-      V = int8_t(U);
-      break;
----------------
kimgr wrote:
> 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?
Yup.


Repository:
  rL LLVM

https://reviews.llvm.org/D50623