[llvm] r330079 - [WebAssembly] Fix a bug in MachineBasicBlock::findDebugLoc() call

[Heejin Ahn via llvm-commits]

That makes sense. Thank you! I submitted https://reviews.llvm.org/D45711.

On Mon, Apr 16, 2018 at 5:11 PM David Blaikie <dblaikie at gmail.com> wrote:

> On Mon, Apr 16, 2018 at 5:07 PM Heejin Ahn <aheejin at gmail.com> wrote:
>
>> Not sure what kind of assertion you are talking about. The invalid IR I
>> was testing with cannot be generated in the current compiler.
>>
>
> Then that's what I mean - assert that the IR doesn't look like the invalid
> thing. Rather than handling the invalid thing in some way.
>
>
>>
>> Oh, and I was not very precise in the previous email: that kind of IR is
>> not gonna be valid IR; that kind of IR will be prevented anyway after
>> adding EH support, because BB sorting routine will be taking EH CFG into
>> consideration.
>>
>> On Mon, Apr 16, 2018 at 4:53 PM David Blaikie <dblaikie at gmail.com> wrote:
>>
>>> Assertion until it can be tested, perhaps? Not sure.
>>>
>>> On Mon, Apr 16, 2018 at 4:51 PM Heejin Ahn <aheejin at gmail.com> wrote:
>>>
>>>> It's soon to be valid IR actually. I'm adding exception handling
>>>> support to wasm now and that kind of IR can be valid soon (within a couple
>>>> weeks). But I can't add it right now because the EH support is not there.
>>>> But without this patch I couldn't even land any other patches.
>>>>
>>>> On Mon, Apr 16, 2018 at 3:54 PM David Blaikie <dblaikie at gmail.com>
>>>> wrote:
>>>>
>>>>> If it's untestable and only impacts invalid IR, maybe an assertion
>>>>> would be better? (& then your invalid IR would fail sooner)
>>>>>
>>>>> On Mon, Apr 16, 2018 at 12:15 PM Heejin Ahn <aheejin at gmail.com> wrote:
>>>>>
>>>>>> It's hard to provide a test case that makes sense, because the
>>>>>> failing condition only happens when a basic block does not have a
>>>>>> terminator but branches to another BB that is not a layout successor (= a
>>>>>> BB that comes right after the current BB), which is not possible. I
>>>>>> accidentally found that bug while debugging my buggy and not-well-formed
>>>>>> IR, which cannot be generated from a valid ll file. The fix was very
>>>>>> trivial, almost to the extent of a typo fix, and the reviewer of that patch
>>>>>> (yurydelendik@) was who wrote that line of the code a month ago, so
>>>>>> I thought he could make sure it was correct. I CC'ed him here.
>>>>>>
>>>>>> On Mon, Apr 16, 2018 at 9:41 AM David Blaikie <dblaikie at gmail.com>
>>>>>> wrote:
>>>>>>
>>>>>>> Test case?
>>>>>>>
>>>>>>> On Fri, Apr 13, 2018 at 5:15 PM Heejin Ahn via llvm-commits <
>>>>>>> llvm-commits at lists.llvm.org> wrote:
>>>>>>>
>>>>>>>> Author: aheejin
>>>>>>>> Date: Fri Apr 13 17:12:12 2018
>>>>>>>> New Revision: 330079
>>>>>>>>
>>>>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=330079&view=rev
>>>>>>>> Log:
>>>>>>>> [WebAssembly] Fix a bug in MachineBasicBlock::findDebugLoc() call
>>>>>>>>
>>>>>>>> Summary:
>>>>>>>> InsertPos is within the bacic block `Header`, so `findDebugLoc()`
>>>>>>>> should
>>>>>>>> be called on not `MBB` but `Header` instead.
>>>>>>>>
>>>>>>>> Reviewers: yurydelendik
>>>>>>>>
>>>>>>>> Subscribers: jfb, dschuff, aprantl, sbc100, jgravelle-google,
>>>>>>>> sunfish, JDevlieghere, llvm-commits
>>>>>>>>
>>>>>>>> Differential Revision: https://reviews.llvm.org/D45648
>>>>>>>>
>>>>>>>> Modified:
>>>>>>>>     llvm/trunk/lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp
>>>>>>>>
>>>>>>>> Modified:
>>>>>>>> llvm/trunk/lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp
>>>>>>>> URL:
>>>>>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp?rev=330079&r1=330078&r2=330079&view=diff
>>>>>>>>
>>>>>>>> ==============================================================================
>>>>>>>> --- llvm/trunk/lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp
>>>>>>>> (original)
>>>>>>>> +++ llvm/trunk/lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp
>>>>>>>> Fri Apr 13 17:12:12 2018
>>>>>>>> @@ -147,9 +147,10 @@ static void PlaceBlockMarker(
>>>>>>>>    }
>>>>>>>>
>>>>>>>>    // Add the BLOCK.
>>>>>>>> -  MachineInstr *Begin = BuildMI(*Header, InsertPos,
>>>>>>>> MBB.findDebugLoc(InsertPos),
>>>>>>>> -                                TII.get(WebAssembly::BLOCK))
>>>>>>>> -
>>>>>>>> .addImm(int64_t(WebAssembly::ExprType::Void));
>>>>>>>> +  MachineInstr *Begin =
>>>>>>>> +      BuildMI(*Header, InsertPos, Header->findDebugLoc(InsertPos),
>>>>>>>> +              TII.get(WebAssembly::BLOCK))
>>>>>>>> +          .addImm(int64_t(WebAssembly::ExprType::Void));
>>>>>>>>
>>>>>>>>    // Mark the end of the block.
>>>>>>>>    InsertPos = MBB.begin();
>>>>>>>>
>>>>>>>>
>>>>>>>> _______________________________________________
>>>>>>>> llvm-commits mailing list
>>>>>>>> llvm-commits at lists.llvm.org
>>>>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>>>>>>>
>>>>>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180417/038e6e01/attachment.html>