[PATCH] D80907: [PowerPC] refactor convertToImmediateForm - NFC

[Qing Shan Zhang via Phabricator via llvm-commits]

steven.zhang added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:3421
+    // Set the bits (        MB + 32        ) to (        ME + 32        ).
+    uint64_t Mask = ((1LLU << (32 - MB)) - 1) & ~((1LLU << (31 - ME)) - 1);
+    InVal &= Mask;
----------------
shchenz wrote:
> steven.zhang wrote:
> > shchenz wrote:
> > > shchenz wrote:
> > > > steven.zhang wrote:
> > > > > It can be improved with APInt utility.
> > > > ```
> > > >  APInt Mask = APInt::getBitsSetWithWrap(32, 32 - ME - 1, 32 - MB);
> > > >  // Current APInt::getBitsSetWithWrap sets all bits to 0 if loBit is equal to highBit.
> > > >  // If MB - ME == 1, we expect a full set mask instead of 0.
> > > >  if (MBSrc - MESrc == 1)
> > > >     Mask.setAllBits();
> > > > ```
> > > > 
> > > > Personally, I think using a raw APInt would be better than above statements?
> > > Should be a raw int... not raw APInt
> > If MB - ME == 1, the current implementation is still zero.
> > ```
> > #include <stdio.h>
> > 
> > int main() {
> >          long long ME=23;
> >          long long MB = ME + 1;
> >          unsigned long long Mask = ((1LLU << (32 - MB)) - 1) & ~((1LLU << (31 - ME)) - 1);
> >          printf("%llx\n", Mask);
> >          return 0;
> > }
> > ```
> > clang test.c;./a.out
> > 0
> OK, thanks for pointing it out. Then the mask calculated here should be a bug. If MB - ME == 1, we expect full mask instead of zero mask.
> 
> I suggest we don't fix this in this NFC patch. This patch intends to make the `convertToImmediateForm` light and mostly move codes from  `convertToImmediateForm` to new created function `SimplifyToLI`.
Agree.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80907