[PATCH] D73664: [PowerPC][Future] Add Support For Functions That Do Not Use A TOC.

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/MCTargetDesc/PPCMCTargetDesc.cpp:241
+          LocalOffset->getLoc(),
+          ".localentry expression is not a valid power of 2.");
+    case 0:
----------------
anil9 wrote:
> Nit : The message is fine just that we are allowing 0 which is not a power of 2, and we are not allowing 2 which is a power of 2. In the second case we will be seeing this message. It is tempting to bring up the relation of power of 2 to this code, but actually it does not exist so it is better not to talk about power of 2 here. This got me confused too in the beginning. 
Please note the comment elsewhere - the message is the message the GNU assembler produces for the same case.


================
Comment at: llvm/lib/Target/PowerPC/MCTargetDesc/PPCMCTargetDesc.cpp:242
+          ".localentry expression is not a valid power of 2.");
+    case 0:
+      return 0;
----------------
anil9 wrote:
> nemanjai wrote:
> > I believe the cases can be rewritten more concisely as:
> > ```
> > case 0:
> > case 1:
> > case 4:
> > case 8:
> > case 16:
> > case 32:
> > case 64:
> >   return Log2(Offset) << ELF::STO_PPC64_LOCAL_BIT;
> > ```
> > That way we still only allow the expected values and we have a nice simple expression for what we return.
> Lo2 wont work here, Log2 of 0 will be negative infinity and of 1 will be zero. 
> 
Ah, of course. Good point. It would have to be:
```
case 0: return 0;
case 1:
case 4:
case 8:
case 16:
case 32:
case 64:
  return Log2(Offset) << ELF::STO_PPC64_LOCAL_BIT;
```


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

https://reviews.llvm.org/D73664