[PATCH] D41872: [MIR] Update MIRLangRef with bundled instructions

[Francis Visoiu Mistrih via Phabricator via llvm-commits]

thegameg added inline comments.


================
Comment at: docs/MIRLangRef.rst:393-395
+where the first instruction ``%r0 = IMPLICIT_DEF`` is **starting** the bundle,
+but it is **outside** the bundle. The two other instructions are **inside** the
+bundle. The ``}`` ends the bundle.
----------------
MatzeB wrote:
> Ugh, I never noticed the MI syntax makes a distinction between "inside" and "outside" of a bundle (I assume that notion comes from `MachineInstr::isInsideBundle()`). Unfortunately that notion doesn't always make sense as we can have bundles without a header. In which case the current syntax can be somewhat confusing :-(
> 
> Because of this I would recommend to not talk about "instructions starting a bundle" or "being outside/inside of a bundle" to not confuse the matter even more. Maybe something like this instead:
> ```
> The first instruction is often a bundle header. The instructions between ``{`` and ``}`` are bundled with the first instruction.
> ```
> 
> I would tweak the example a bit to be something that would actually be valid MI:
> ```
> BUNDLE implicit-def %r0, implicit-def %r1, implicit %r2 {
>   %r0 = SOME_OP %r2
>   %r1 = ANOTHER_OP internal %0
> }
> ```
Thanks, makes more sense.


================
Comment at: docs/MIRLangRef.rst:397-404
+The following syntax where the first instruction **inside** the bundle is on
+the same line is also accepted:
+
+.. code-block:: text
+
+    %r0 = IMPLICIT_DEF { %r1 = IMPLICIT_DEF
+      %r2 = IMPLICIT_DEF
----------------
MatzeB wrote:
> Did this syntax emerge by accident (I don't see what it is good for)? Should we maybe not document it?
We explicitly handle this case in [[ https://github.com/llvm-mirror/llvm/blob/55c93e1f45f9cdf784913f2cf7213208581f6b5d/lib/CodeGen/MIRParser/MIParser.cpp#L647 |  the MIR parser ]], but I can't see why. Should we get rid of it?


https://reviews.llvm.org/D41872