[InstCombine] Avoid passing pow attributes to sqrt

[Saldivarcher]

Summary

As described on this issue, instcombine seems to be passing the attributes of pow to sqrt. This in turn causes an assertion to be hit later on.

Here is how to get the assertion:

$ cat small.ll 
define void @PR58475(double %x) {
  %call = call afn double @pow(double %x, double 1.5)
  ret void
}

declare double @pow(double noundef, double noundef)
$ opt -instcombine small.ll -disable-output

[spatel]

Please add the crashing example as a regression test. I'm not sure if we can modify an existing file to show the crash without changing a bunch of other tests, so it can be a new file in the llvm-project/llvm/test/Transforms/InstCombine dir.

llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp

2102

If we don't need the Attrs for any other transforms, it would be better to declare/init it just above the code where it is used.

[Saldivarcher]

Okay, test was added, hopefully it looks reasonable. Let me know if I need to change/update anything.

[Saldivarcher]

llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp

2102

Just seen this, let me declare above the call.

[spatel]

llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp

2166

Do we need to name the default/empty AttributeList if this is the only use?
Ie, can the code be shorter?

Sqrt = getSqrtCall(Base, AttributeList(), Pow->doesNotAccessMemory(), M, B, TLI);

[spatel]

llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp

2166

I tried to make the existing code around here easier to understand with:
bff6880a5f89

[Saldivarcher]

llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp

2166

Your changes LGTM, I like the Attrs -> NoAttrs change, really helps with readability. The AttributeList() change is alright as well. Should this change just include the test then? Or should I close it completely?

[spatel]

llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp

2166

I left this case alone, so there wouldn't be a conflict. You can either name the variable "NoAttrs" or default construct it in the getSqrtCall(), and then we should be good. Do you have commit access?

[Saldivarcher]

llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp

2166

Oops, I misread your commit, I thought you handled this case! But okay, I updated the code to use AttributeList(). No, I don't have commit access :( Can you commit this for me?

[spatel]

LGTM

llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp

Value *LibCallSimplifier::optimizePow(CallInst *Pow, IRBuilderBase &B) {
   Value *Base = Pow->getArgOperand(0);
   Value *Expo = Pow->getArgOperand(1);
   Function *Callee = Pow->getCalledFunction();
   StringRef Name = Callee->getName();
   Type *Ty = Pow->getType();
   Module *M = Pow->getModule();
   bool AllowApprox = Pow->hasApproxFunc();
   bool Ignored;
+  AttributeList Attrs; // Attributes are only meaningful on the original call
 
   // Propagate the math semantics from the call to any created instructions.
   IRBuilderBase::FastMathFlagGuard Guard(B);
   B.setFastMathFlags(Pow->getFastMathFlags());
   // Evaluate special cases related to the base.
 
   // pow(1.0, x) -> 1.0
   if (match(Base, m_FPOne()))
if (!ExpoA.isInteger()) {
 
       if (ExpoI.roundToIntegral(APFloat::rmTowardNegative) !=
           APFloat::opInexact)
         return nullptr;
       if (!ExpoI.isInteger())
         return nullptr;
       ExpoF = &ExpoI;
 
-      Sqrt = getSqrtCall(Base, Pow->getCalledFunction()->getAttributes(),
-                         Pow->doesNotAccessMemory(), M, B, TLI);
+      Sqrt = getSqrtCall(Base, Attrs, Pow->doesNotAccessMemory(), M, B, TLI);
       if (!Sqrt)
         return nullptr;
     }
 
     // 0.5 fraction is now optionally handled.
     // Do pow -> powi for remaining integer exponent
     APSInt IntExpo(TLI->getIntSize(), /*isUnsigned=*/false);
     if (ExpoF->isInteger() &&
         ExpoF->convertToInteger(IntExpo, APFloat::rmTowardZero, &Ignored) ==
             APFloat::opOK) {