[RFC] ExpandMemCmpPass is not prepared for architectures smaller than 32 bit

Code Generation Common CodeGen Infrastructure
1

Hello all,

This is related to the RL78 backend which I want to upstream ([RFC] Upstreaming Renesas RL78 MCU backend)
As suggested there I will discuss all the controversial changes.

As explained before, RL78 is a 16 bit architecture, and a very simple IR example before the ExpandMemCmpPass would look like this(note the i16):

define dso_local i16 @testmemcmp1(ptr nocapture noundef readonly %0, ptr nocapture noundef readonly %1) local_unnamed_addr #0 {
  %3 = tail call i16 @memcmp(ptr noundef nonnull dereferenceable(5) %0, ptr noundef nonnull dereferenceable(5) %1, i16 noundef 5)
  ret i16 %3
}

Left unchanged the IR after the ExpandMemCmpPass would look like the left side below (note the change from i16 to i32).
This is due to the getInt32Ty() being used in multiple locations regardless of the current architecture.
I propose to replace calls (multiple locations) to getInt32Ty() with calls to getIntNTy(IntSize) where IntSize is a member in MemCmpExpansion and it will be initialized in the constructor (MemCmpExpansion::MemCmpExpansion) based on the target architecture (right side).

image
image1787Ă—745 42.5 KB

Is this an acceptable change?

Currently, in my implementation, I initialize “IntSize” as 16 for RL78 and 32 for the rest, I do this based on target triple name.
I assume there might be a better way to do this(maybe based on some information from DataLayout?) and I can investigate this if the change is acceptable.
I also did not check if any target might preferer a 64 bit integer. I can look into this, for completeness, as well.

Thank you!

Best Regards,
Sebastian

2

memcmp() returns a C int type, so it should make use of the TargetLibraryInfo::getIntSize() API. The integer size is initialized based on Triple.isArch16Bit().

So I would expect that if you change ExpandMemCmp to make use of getIntSize() things should “just work” for RL78 as well.

Alternatively, it would also be fine to just use the return type of memcmp, which must be the same.

We already have other 16-bit targets (msp430, avr), so you should be able to land a fix for this independently from the target upstreaming.

1 Like