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

[Eli Friedman via Phabricator via llvm-commits]

efriedma added a comment.

In D123198#3442121 <https://reviews.llvm.org/D123198#3442121>, @nikic wrote:

> If we wanted to make that clearer, we could separate out this part to only add these attributes when creating new libcalls. That should also make it obvious that it's fine that this code doesn't run under `O0`, because we only care about libcalls introduced by optimizations.

This might make sense... I guess we could introduce a separate routine to compute the "mandatory" attributes.  Maybe make it a combined routine, actually; have it call getOrInsertFunction, then compute the mandatory attributes for that function.  That would let us be more strict, also: we could assert() that we don't build new libcalls that don't have the code for computing mandatory attributes.

Then we can say inferLibFuncAttributes is just an optimization (and the emit* utilities call it just for the sake of avoiding pass ordering issues).



================
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;
----------------
jonpa wrote:
> 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..?
I don't think LibFunc_chown/LibFunc_lchown are used anywhere else... so dropping them from TargetLibraryInfo wouldn't have any practical effect.

That said, like @nikic noted, we only really need to add sign/zeroext attributes to new libcalls; existing ones should already have the right attributes.  So we could get away with still recognizing chown/lchown as long as we don't try to construct calls to them.




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

https://reviews.llvm.org/D123198