[PATCH] D63046: [Attributor] Deduce "willreturn" function attribute

[Hideto Ueno via Phabricator via llvm-commits]

uenoku added inline comments.


================
Comment at: llvm/trunk/lib/Transforms/IPO/Attributor.cpp:1284
+       {(unsigned)Instruction::Invoke, (unsigned)Instruction::CallBr,
+        (unsigned)Instruction::Call}) {
+    for (Instruction *I : OpcodeInstMap[Opcode]) {
----------------
nikic wrote:
> uenoku wrote:
> > nikic wrote:
> > > In line with `isGuaranteedToTransferExecutionToSuccessor()`, isn't it also necessary to check for volatile memory operations, which may trap?
> > In D53184, it is clarified in LangRef that volatile memory operations wouldn't trap. Therefore, in my opinion, we need to fix rather `isGuaranteedToTransferExecutionToSuccessor`, isn't it?
> Thanks for pointing that out! In that case indeed the implementation of guaranteed-to-transfer should be changed. Especially the paragraph
> 
> > The compiler may assume execution will continue after a volatile operation,
> > so operations which modify memory or may have undefined behavior can be
> > hoisted past a volatile operation.
> 
> seems pretty clear on this point -- this not only precludes trapping, but also a volatile MMIO operation that hangs forever.
Btw  `AtomicCmpXchgInst` are checked in `isGuaranteedToTransfer..` whether the instruction is volatile. However, I think in some cases `AtomicCmpXchgInst` never return so we need to fix also `isGuaranteedToTransfer..` in this point. For the same reason, `AtomicCmpXchgInst` should be checked in `willreturn` deduction. 


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63046