This is an archive of the discontinued LLVM Phabricator instance.

[IR] Add operand bundles to CallInst and InvokeInst.
ClosedPublic

Authored by sanjoy on Aug 28 2015, 4:24 PM.

Details

Summary

This change teaches CallInsts and InvokeInsts to maintain a set of
operand bundles as part of its operands. CallInsts and
InvokeInsts with operand bundles co-allocate some space before their
Use array to hold meta information
(OperandBundleUser::BundleOpInfo) about which of its operands are
part of an operand bundle.

The strings corresponding to the bundle tags are interned into
LLVMContextImpl::BundleTagCache. We could reference count these if
needed, but right now I don't see any need to do so.

This change does not include any parsing / bitcode support. That's
the next change.

Depends on D12455.

Diff Detail

Event Timeline

sanjoy updated this revision to Diff 33488.Aug 28 2015, 4:24 PM
sanjoy retitled this revision from to [IR] Add operand bundles to CallInst and InvokeInst..
sanjoy updated this object.
sanjoy added a subscriber: llvm-commits.
include/llvm/IR/InstrTypes.h
1108

nit: Why explicit, when we have two parameters?

1131

super-nit: I think Back would be a more common name for this than InclusiveEnd

include/llvm/IR/Instructions.h
1430

Should we pad this to pointer-align the StringMapEntry<uint32_t> *Tag field of the BundleOpInfos (and likewise for the other CallInst::Create overload and the two InvokeInst::Creates)?

lib/IR/LLVMContextImpl.h
989–990

nit: worth a comment?

lib/IR/User.cpp
121–122 ↗(On Diff #33488)

Ok, I should have read ahead :). But I still think it would be good to discuss alignment in the method header comment, and to 8-byte align the CallInst/InvokeInst bundles on 64-bit platforms for the pointer to the string map entry.

sanjoy added inline comments.Aug 31 2015, 7:51 PM
include/llvm/IR/InstrTypes.h
1108

C++11 allows implicit multi-arg constructors -- like X x = {1, 2}.

1131

Will fix.

lib/IR/LLVMContextImpl.h
989–990

Will add.

lib/IR/User.cpp
121–122 ↗(On Diff #33488)

I intended this to be part of the first change, I must have messed up rebasing somewhere. I'll move this bit to D12455 and add a comment, as you suggested.

sanjoy added a subscriber: sanjoy.Aug 31 2015, 9:31 PM

I don't know why this hasn't showed up on llvm-commits yet, but I've
replied to the review on phabricator.

sanjoy updated this revision to Diff 33749.Sep 1 2015, 4:06 PM
sanjoy marked an inline comment as done.

Address review.

sanjoy added inline comments.Sep 1 2015, 4:08 PM
include/llvm/IR/Instructions.h
1430

Please do check my math, but after this current round of changes, I think the Tag field should be aligned. Now, the layout of a User (in 64bit platforms) with a BundleOpInfo should be

BundleOpInfo[0]    // addr is 8x
  Tag              // addr is 8x
  uint32_t         // addr is 8(x + 1)
  uint32_t         // addr is 8(x + 1) + 4
BundleOpInfo[1]    // addr is 8(x + 2)
  ...
... BundleOpInfo[1, 2, 3] ...
DescriptorInfo     // addr is 8y
  intptr_t         // addr is 8y
Use[]              // addr is 8(y + 1)
  ...
User               // addr is 8(y + 1 + 3 * # uses)

For 32 bit platforms, things should roughly stay the same, except the uint32_ts will take up one whole word instead.

However, I'm sure if the Begin and End fields should be uint32_ts, instead of being a native int. What do you think?

MatzeB added a subscriber: MatzeB.Sep 1 2015, 4:52 PM

Would be good to document somewhere what an operand bundle is and maybe the motivation or an example use case for it.

sanjoy added a comment.Sep 1 2015, 5:07 PM

This is the original discussion:
http://lists.llvm.org/pipermail/llvm-dev/2015-August/089070.html (I
should have included it with the patch to begin with).

I'll update the langref with more information in the next revision.

JosephTremoulet added inline comments.Sep 1 2015, 5:58 PM
include/llvm/IR/Instructions.h
1430

I agree with your math. I think leaving Begin and End as uint32_t makes sense; I doubt this would be the only weak link if someone tried to put 2^32 operands on a call, and even if we were to run on a platform with an odd pointer size, I'd expect sizeof(BundleOpInfo) to be a multiple of it by virtue of the Tag pointer field.

I'll update the langref with more information in the next revision

(in case others get confused looking for it here like I did): the langref update went into D12457.

sanjoy updated this revision to Diff 33865.Sep 2 2015, 3:19 PM
sanjoy edited edge metadata.
  • address @majnemer's off-line review comment -- use an ArrayRef<> instead of a std::pair for getDescriptor()
sanjoy updated this revision to Diff 34298.Sep 8 2015, 9:54 PM
  • add more comments to OperandBundleUser
  • move Use& population logic to populateBundleOperandInfos to keep everything in one place.
sanjoy updated this revision to Diff 34402.Sep 9 2015, 7:03 PM
  • address Duncan's review
sanjoy updated this revision to Diff 34597.Sep 11 2015, 3:45 PM
  • Some changes here to support changes in D12457: mainly add accessors around LLVMContextImpl::BundleTagCache.
sanjoy updated this revision to Diff 34752.Sep 14 2015, 3:56 PM
  • Supporting changes to support changes to D12457: have OperandBundleDef own the data it contains, by using std::vector and std::string instead of ArrayRef and StringRef
  • Unbreak the build with GCC, by correctly using the ArrayRef<> in OperandBundleUse
  • Remove 'Set' from OperandBundleSetDef and OperandBundleSetUse
This revision was automatically updated to reflect the committed changes.

Just to make it clear: this was LGTM'ed by Duncan via email (for some reason these emails did not show up here).

Sanjoy,

Your comment in include/llvm/IR/InstrTypes.h in line 1137 is undeniably
nice, but it causes a warning (or an error when building with "-Wall"):

“error: multi-line comment [-Werror=comment]”

Could you please fix it? For example you can use | instead of \, or add
some "shielding" (|, ///, or whatever you prefer) in the leftmost position
of the whole pseudo graphics (i.e. lines 1137-1149.

Thanks!

Dmitry.

Dmitry Babokin via llvm-commits wrote:
> dbabokin added a subscriber: dbabokin.
> dbabokin added a comment.
>
> Sanjoy,
>
> Your comment in include/llvm/IR/InstrTypes.h in line 1137 is undeniably
> nice, but it causes a warning (or an error when building with "-Wall"):
>
> “error: multi-line comment [-Werror=comment]”
>
> Could you please fix it? For example you can use | instead of , or add
> some "shielding" (|, ///, or whatever you prefer) in the leftmost
position
> of the whole pseudo graphics (i.e. lines 1137-1149.

I'm more than happy to fix this, but I'm not able to reproduce this
issue using clang++ on OSX (my default build environment).

Can you please verify that the change below fixes the issue?

diff --git a/include/llvm/IR/InstrTypes.h b/include/llvm/IR/InstrTypes.h
index 70ab973..0c2ff91 100644

  • a/include/llvm/IR/InstrTypes.h

+++ b/include/llvm/IR/InstrTypes.h
@@ -1134,19 +1134,19 @@ typedef OperandBundleDefT<const Value *>
ConstOperandBundleDef;

///
/// The layout of an operand bundle user is
///

-/ +-------uint32_t End---------------------------------+
-
/ /
-/// / +------uint32_t Begin------------------+

-///           /  / 
  +///          +-----------uint32_t

End-------------------------------------+
+/ | |
+
/ | +--------uint32_t Begin--------------------+ |
+/// | | | |

///          ^  ^                                          v 
   v
///
///  | BOI0 | BOI1 | .. | DU | U0 | U1 | .. | BOI0_U0 | .. | BOI1_U0 |

.. | Un

///
///   v  v                                  ^              ^

-/ / /
-
/ +------uint32_t Begin----------+ /
-/ /
-
/ +-------uint32_t End-------------------------+
+/ | | | |
+
/ | +--------uint32_t Begin------------+ |
+/ | |
+
/ +-----------uint32_t End-----------------------------+

///
///
/// BOI0, BOI1 ... are descriptions of operand bundles in this User's

use list.

>
> Thanks!
>
> Dmitry.
>
>
> Repository:
> rL LLVM
>
> http://reviews.llvm.org/D12456
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits

Sanjoy Das wrote:

sanjoy added a comment.

Dmitry Babokin via llvm-commits wrote:

>  dbabokin added a subscriber: dbabokin.
>  dbabokin added a comment.
>
>  Sanjoy,
>
>  Your comment in include/llvm/IR/InstrTypes.h in line 1137 is undeniably
>  nice, but it causes a warning (or an error when building with "-Wall"):
>
>  “error: multi-line comment [-Werror=comment]”
>
>  Could you please fix it? For example you can use | instead of , or add
>  some "shielding" (|, ///, or whatever you prefer) in the leftmost

position

>  of the whole pseudo graphics (i.e. lines 1137-1149.

I'm more than happy to fix this, but I'm not able to reproduce this
issue using clang++ on OSX (my default build environment).

Can you please verify that the change below fixes the issue?

Looks like my mail client screwed up the diff. Sending it as an
attachment this time.

diff --git a/include/llvm/IR/InstrTypes.h b/include/llvm/IR/InstrTypes.h
index 70ab973..0c2ff91 100644

  • a/include/llvm/IR/InstrTypes.h

+++ b/include/llvm/IR/InstrTypes.h
@@ -1134,19 +1134,19 @@ typedef OperandBundleDefT<const Value *>
ConstOperandBundleDef;

///
/// The layout of an operand bundle user is
///

-/ +-------uint32_t End---------------------------------+
-
/ /
-/// / +------uint32_t Begin------------------+

-///           /  /
  +///          +-----------uint32_t

End-------------------------------------+
+/ | |
+
/ | +--------uint32_t Begin--------------------+ |
+/// | | | |

///          ^  ^                                          v
   v
///
///  | BOI0 | BOI1 | .. | DU | U0 | U1 | .. | BOI0_U0 | .. | BOI1_U0 |

.. | Un

///
///   v  v                                  ^              ^

-/ / /
-
/ +------uint32_t Begin----------+ /
-/ /
-
/ +-------uint32_t End-------------------------+
+/ | | | |
+
/ | +--------uint32_t Begin------------+ |
+/ | |
+
/ +-----------uint32_t End-----------------------------+

///
///
/// BOI0, BOI1 ... are descriptions of operand bundles in this User's

use list.

>  Thanks!
>
>  Dmitry.
>
>
>  Repository:
>     http://reviews.llvm.org/diffusion/L/ LLVM
>
>  http://reviews.llvm.org/D12456
>
>
>
>  _______________________________________________
>  llvm-commits mailing list
>  llvm-commits@lists.llvm.org
>  http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits

Repository:

rL LLVM

http://reviews.llvm.org/D12456

Sanjoy,

Thanks for the fix, it works.

I forgot to mention that the problem shows up when building with gcc (we
use 4.9.1). We have to use gcc on Linux when building our project, which
includes LLVM libraries and hence we include include/llvm/* stuff.

Thanks again for quick response.

sanjoy added a comment.Oct 2 2015, 4:27 PM

Dmitry Babokin via llvm-commits wrote:

dbabokin added a comment.

Sanjoy,

Thanks for the fix, it works.

Checked in at r249212.

  • Sanjoy

I forgot to mention that the problem shows up when building with gcc (we
use 4.9.1). We have to use gcc on Linux when building our project, which
includes LLVM libraries and hence we include include/llvm/* stuff.

Thanks again for quick response.

Repository:

rL LLVM

http://reviews.llvm.org/D12456


llvm-commits mailing list
llvm-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits