[PATCH] D98780: [IR] Add opt-in flag to isIndirectCall() to consider inlineasm

[Madhur Amilkanthwar via Phabricator via llvm-commits]

madhur13490 requested review of this revision.
madhur13490 added inline comments.


================
Comment at: llvm/lib/IR/Instructions.cpp:289
     return false;
-  return !isInlineAsm();
+  return InlineAsmMayHaveIndirectCall == isInlineAsm();
 }
----------------
rampitec wrote:
> madhur13490 wrote:
> > madhur13490 wrote:
> > > rampitec wrote:
> > > > madhur13490 wrote:
> > > > > rampitec wrote:
> > > > > > It will give wrong answer for non-asm. "return !isInlineAsm() || !InlineAsmMayHaveIndirectCall;"
> > > > > I don't think why it would return wrong answer. Here is the truth table of the operation:
> > > > > 
> > > > > 1. InlineAsmMayHaveIndirectCall = false, isInlineAsm() = false --> return "true" (as expected)
> > > > > 2. InlineAsmMayHaveIndirectCall = false, isInlineAsm() = true --> return "false" (as expected)
> > > > > 3. InlineAsmMayHaveIndirectCall = true, isInlineAsm() = true --> return "true" (as expected)
> > > > > 4. InlineAsmMayHaveIndirectCall = true, isInlineAsm() = false --> return "false" (as expected)
> > > > > 
> > > > > 1 & 2 above are according to today's behavior. 3 & 4 are the new ones. Which of the above is incorrect?
> > > > > 
> > > > > FWIW, "==" operator enacts XNOR for bools.
> > > > > 
> > > > Last case is incorrect. If it is not isInlineAsm() we should return true, just like now. It does not matter what inline asm may or may not contain then.
> > > > 
> > > > But my condition is also wrong. I think it is "return !isInlineAsm() || InlineAsmMayHaveIndirectCall;".
> > > No, if you're passing inlineAsmMayHaveIndirectCall = true then you're considering that the statement might have one inside the block box.
> > > 
> > > So, it essentially means "isInlineAsm() == presence of indirect call". If inlineAsm() = true then you have indirect call (condition 3). If inlineAsm() = false then you don't have indirect call (condition 4). 
> > > 
> > > The function should either choose to NOT consider anything about inlineAsm() or should provide flexibility to consider. In latter case, it is not useful and we have to check !callee everywhere. 
> > > No, if you're passing inlineAsmMayHaveIndirectCall = true then you're considering that the statement might have one inside the block box.
> > > 
> > > So, it essentially means "isInlineAsm() == presence of indirect call". If inlineAsm() = true then you have indirect call (condition 3). If inlineAsm() = false then you don't have indirect call (condition 4). 
> > > 
> > > The function should either choose to NOT consider anything about inlineAsm() or should provide flexibility to consider. In latter case, it is not useful and we have to check !callee everywhere. 
> > 
> > Typo: In latter case, it is useful than to check !callee everywhere.
> > 
> Right now it considers anything which is non inline asm got to this point as an indirect call. You are breaking it. Option wrt inline asm shall not affect non inline asm.
InlineAsmMayHaveIndirectCall is "false" by default so no breakage and the function continue to work as it is today for all users (condition1 and 2). If you pass "true" then you get new behavior which this patch proposes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98780