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

[Jonas Paulsson via Phabricator via llvm-commits]

jonpa added inline comments.


================
Comment at: llvm/include/llvm/Transforms/Utils/BuildLibCalls.h:38
+  FunctionCallee getOrInsertLibFunc(Module *M, const TargetLibraryInfo &TLI,
+                                LibFunc TheLibFunc,  StringRef Name,
+                                FunctionType *T, AttributeList AttributeList);
----------------
efriedma wrote:
> Does getOrInsertLibFunc really need a "Name" argument?  We have TargetLibraryInfo; we can just look up the correct name.
I removed it and it seems that it is nearly identical, but for one test case:

double-float-shrink-1.ll

```
-; MS64-NEXT:     [[LOGBF:%.*]] = call fast float @logbf(float [[F:%.*]])
+; MS64-NEXT:     [[LOGBF:%.*]] = call fast float @_logbf(float [[F:%.*]])
```

On this target the name gets a different name with 'TLI.setAvailableWithName(LibFunc_logbf, "_logbf");'.

I am not sure what the difference here means, but I am hoping that it would work to make that call to the preferred version directly, so I kept this change and updated the test.




================
Comment at: llvm/lib/Transforms/Utils/BuildLibCalls.cpp:1238
+  // Make sure that any narrow integer argument is extended if required by
+  // the target ABI.
+  if (TLI.getExtAttrForI32Param() != Attribute::None)
----------------
efriedma wrote:
> I'd prefer to make the assertion target-independent if possible; if someone is adding a new libcall optimization, they might not remember to write a test specifically for SystemZ.
> 
> Can we just add a check in the "default" case that the call doesn't pass/return any integer values, or something like that?  (I guess in that case, you'd have to list out all the calls we build that have a size_t argument or return value, but that should be a much smaller list than every function supported by inferNonMandatoryLibFuncAttrs.)
Ah, that seems to work: if we can assume that size_t never needs extension (which I "hope") we get only a small list of functions here to exclude and can have the check enabled for all targets...



================
Comment at: llvm/test/Transforms/InstCombine/strchr-1.ll:3
 ; Test that the strchr library call simplifier works correctly.
 ; RUN: opt < %s -passes=instcombine -S | FileCheck %s
+; RUN: opt < %s -passes=instcombine -S -mtriple=systemz-unknown | FileCheck %s -check-prefixes=CHECK-SYSTEMZ
----------------
efriedma wrote:
> jonpa wrote:
> > Will this test pass on all targets or should the first line here have a target triple to match the output (uses of i64)...?
> > Or perhaps running it just once on SystemZ would work?
> > 
> > (fputs-1.ll same)
> > 
> > 
> In general, if you're going to manually hack at tests with an "Assertions have been autogenerated" marking, please either regenerate them or drop the marking.
> 
> These tests were written with the assumption the generated IR wouldn't be target-specific.  Since it turns out the input and result are in fact target-specific, we should just pick a target, and shove them into the relevant target-specific subdirectory (llvm/test/Transforms/InstCombine/X86/ etc.).  And maybe add a few tests to llvm/test/Transforms/InstCombine/SystemZ/ to confirm the whole signext marking mechanism is working.
> 
> Alternatively, you could try to hack at the tests to try make them agnostic to whether there's a signext marking, but that seems like extra work for not much benefit.
ok - I backed out of those tests and instead made a new SystemZ test file.

I think those tests don't have CHECKs for any target-specific result, so leaving them as is under /InstCombine.




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

https://reviews.llvm.org/D123198