Miri and miri-related code contains repetitions of (n << amt) >> amt

[oli-obk]

All of these should be deduplicated under a common set of functions that do sign extension or truncation of integers.

The functions should live somewhere in librustc, so probably librustc::mir::interpret, because they are also needed in librustc::ty::layout.

The parts that need to be figured out:

• what information is needed in each function (so we don't have to prepend all function calls with an "almost the same" computation of e.g. type layouts.
• are there many uses of sign extension followed by truncation and should these be a single function, too?

[stevepentland]

This sounds pretty interesting and I may like to take it on, but I don't really understand the scope of what will be changed at the moment.

[oli-obk]

@stevepentland the goal is to reduce code repetition. Essentially all code that looks similar to the title code should be calling dedicated functions for either sign or zero extension. These methods already exist on the EvalContext, but we need to move them to rustc::mir::interpret so we can also use them for the code in the file that declares struct Discr<'tcx>.

[QuestofIranon]

I'm interested in taking this on if anything still needs to be done for it.

[oli-obk]

I think we have the appropriate functions now, but we aren't using them everywhere, and they don't live in librustc yet (they are still in rustc_mir::interpret)

[kostrowski]

I believe these are all the locations that do sign extensions or truncates:

• src/librustc_lint/types.rs, line 379
• src/librustc_lint/types.rs, line 385
• src/librustc_mir/hair/cx/mod.rs, line 157
• src/librustc_mir/interpret/eval_context.rs, line 952
• src/librustc_mir/interpret/eval_context.rs, line 963
• src/librustc_mir/interpret/eval_context.rs, line 1026
• src/librustc/ty/layout.rs, line 896
• src/librustc/ty/sty.rs, line 1910
• src/librustc/ty/util.rs, line 50
• src/librustc/ty/util.rs, line 75 (in a closure that is then used in lines 77 and 79)
• src/librustc/ty/util.rs, line 90

In each case the operations are preceded by usage of either rustc_target::abi::TyLayout via fn TyCtxt::layout_of(...) or rustc_target::abi::Integer via fn IntegerExt::from_attr(...) to get the size of the type that's needed to perform the bit shifts.

Since an Integer can be extracted from a TyLayout for integer layouts, I believe that the best location for the sign_extend and truncate functions would be as instance methods on Integer. The instance methods internally would have access to the size, so we could de-duplicate quite a bit of code at each call site.

I think there are two options for where to locate the method implementations:

1. Within the impl block here in 'librustc_target.
2. Within the impl IntegerExt for Integer block here in librust.

I'm happy to work on a pull request if this seems like a reasonable way to refactor. I haven't contributed to the project before, so might need some guidance.

[oli-obk]

Great writeup of the current state!

I think it can live directly in the impl block, because it's not something that requires any types beyond u128.

How verbose and common would it be to need to extract the Integer from the TyLayout? Maybe a helper method on TyLayout could make sense.

I'm happy to work on a pull request if this seems like a reasonable way to refactor. I haven't contributed to the project before, so might need some guidance.

Feel free to ask anything here or on discord/zulip/irc. General build instructions can be found at https://github.com/rust-lang/rust#building-from-source and https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md#the-build-system

I also can advise to set incremental = true in the config.toml (file is described in https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md#configuration). Incremental will speed up your build process a lot, even if tests run a little slower then.

[kostrowski]

Okay, I'll take a look at it and hopefully have something shortly. Thank you for the tip about incremental = true; I've been building okay but didn't have that setting.

How verbose and common would it be to need to extract the Integer from the TyLayout? Maybe a helper method on TyLayout could make sense.

I think it has to peel back enough layers of TyLayout to warrant a helper method. There are already helper methods on both TyLayout and Abi. Abi has is_unsized and is_signed while TyLayout has is_unsized (a simple wrapper around Abi::is_unsized) and is_zst. Given that TyLayout derefs to LayoutDetails which has the abi: Abi field, it really isn't much different for the call sites to use one or the other (e.g. either layout.as_integer() or layout.abi.as_integer()).

I figure the helper method (as_integer()?) can return an Option<Integer> and let the call sites decide what to do when the layout isn't for an integer (likely just panic on unwrapping).

[oli-obk]

related recent changes: https://github.com/rust-lang/rust/pull/53424/files#diff-be07fb44ceb7024a91292d9680398e5cR581

[kenta7777]

I'd like to tackle this issue. How shall we begin?

[oli-obk]

I am actually not sure right now. There were a lot of changes to the compiler.

You can start by going through this list and seeing whether there are any left that need to be addressed

[kenta7777]

@oli-obk Thank you for your advice. I' ll try.

[varkor]

How shall we begin?

There are also some comments // FIXME(49937) in the code that you should take a look at.

[kenta7777]

@varkor Sorry for the delay in replying. Thanks!

[kenta7777]

I think Reducing the repetition of this list has almost finished.
Are the remaining repetition marked by // FIXME(49937) only?

[oli-obk]

I think Reducing the repetition of this list has almost finished

Awesome! Thank you

Are the remaining repetition marked by // FIXME(49937) only
?

There might be more, but I'm fine closing this issue after the known ones are done.

[kenta7777]

I understand. I'll keep working on this issue until the repetition of // FIXME(49937) codes are reduced.

[kenta7777]

@oli-obk Could you open this issue because codes which should be fixed still remain?

[kenta7777]

Thanks!

[kenta7777]

I think refactoring codes marked as FIXME(49937) was done.

[oli-obk]

Super! Thanks for all the work you put into cleaning up this issue!