[RFC] Unify kDynamicSize and kDynamicStrideOrOffset

MLIR
1

There are two constants in mlir::ShapedType that can be unified into one.

static constexpr int64_t kDynamicSize = -1;
static constexpr int64_t kDynamicStrideOrOffset 
  = std::numeric_limits<int64_t>::min();

It can be done in two steps. The first PR sets kDynamicSize to kDynamicStrideOrOffset. That should surface any cases when -1 was hardcoded.

The second PR would replace both kDynamicSize and kDynamicStrideOrOffset with kDynamicValue or kDynamicSentinel or kDynamicPlaceholder. Please, suggest better names.

2 Likes
2

This would be a highly useful cleanup!

1 Like
3

Thanks, this would be a helpful improvement. If there are no objections, for the first step, can you post the patch here and we let it soak for ~a week so that we can check downstreams. I don’t anticipate issues, but I am a bit concerned by these values having made their way into things that we don’t expect.

4

+1, I was just looking at that this week too (and yes there are parts hardcoded). I’d even be curious about flushing out hard coding of both of these and picking some other negative value initially (but the -1 is much easier one that slips in)

5

Yes this is one of the longstanding cleanups.

The main reason it was not done before is because of pervasive -1 magic constant usage inherited from early affine code.

My experience last time I tried the first step of your list (c.a. EoY 2019) was that we had a lot of subtle issues popping up in internal clients that use MLIR (I won’t mention names :slight_smile: ).

The situation may have improved these days, so big +1 from me.

6

Let’s use a random value generated at build time :wink:

More seriously, int_min LGTM!

7

rG9729b6930b41 landed. This is the first part that changed kDynamicSize to int_min. The second part unifies the constants. ⚙ D138282 Merge kDynamicSize and kDynamicSentinel into one constant.

1 Like
8

Awesome!

… PSA! PSA!