[PATCH] D123198: [LibCalls] Add argument extension attributes to more functions.

[Jonas Paulsson via Phabricator via llvm-commits]

jonpa added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/BuildLibCalls.cpp:607
+    Changed |= setArgExtAttr(F, 1, TLI, false/*Signed*/);
+    Changed |= setArgExtAttr(F, 2, TLI, false/*Signed*/);
+    LLVM_FALLTHROUGH;
----------------
xbolva00 wrote:
> jonpa wrote:
> > xbolva00 wrote:
> > > jonpa wrote:
> > > > efriedma wrote:
> > > > > Are uid_t/gid_t unsigned on all targets?
> > > > > 
> > > > > (I wouldn't mind just dropping LibFunc recognition for chown etc. if necessary...)
> > > > I see your point: since uid_t/gid_t (as far as I could see) are either unsigned or signed we cannot have a generic BuildLibCall utility for them with the current implementation.
> > > > 
> > > > If this is needed it seems to me we would need to add a target based flag similar to ShouldExtI32Param for this to properly determine the types of uid_t and gid_t.
> > > > 
> > > > All tests are however now passing after removing chown and lchown, so I did this instead now per your suggestion.
> > > > 
> > > Why do you need to drop it all? You added nothing so how this blocks you now?
> > It doesn't seem right to keep incorrect prototypes around if they are not fixed.
> > 
> So it would be better to fix it properly.
> 
> Somebody in the future may reintroduce handling of chown…
> 
Eli wrote he thought we could remove them which made sense to me given the issue with uid_t/gid_t...

Do you see a need for them here to motivate adding a recogniziton of uid_t/gid_t types signedness per target?

Since no tests fail if they are removed (other than the immediate annotate.ll test), I thought this could be done later if needed...
 
@efriedma : I now actually wonder if you meant that these functions be removed also in  TargetLibraryInfo.def and other places..?


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

https://reviews.llvm.org/D123198