Page MenuHomePhabricator

Change X86RetpolineThunks from a MachineFunctionPass to a ModulePass
AbandonedPublic

Authored by sconstab on Mon, Mar 23, 1:00 PM.

Details

Summary

X86RetpolineThunks had been implemented as a MachineFunctionPass. Its behavior is to examine each function in a module, and determine whether that function requires the X86 retpoline feature. If so, retpoline thunk(s) are inserted into the module, if they have not been inserted already. This behavior seems to make more sense as a ModulePass.

In this patch, runOnModule() iterates through all of the functions in the module. As soon as a function requiring retpolines is encountered, the retpoline thunk is inserted and populated. This is probably more efficient than the prior implementation, and it also does not require the pass to carry around any internal state.

One consequence of making this a ModulePass is that it seems to affect the ordering of suffix generation for temporary symbols. Therefore some regression tests needed to be lightly edited.

Diff Detail

Event Timeline

sconstab created this revision.Mon, Mar 23, 1:00 PM

This probably has a hidden side effect on the pass manager construction. Inserting a module pass into the middle of the pipeline effectively introduces a serialization point in the middle of the pipeline. This means all functions have to reach this point in the pipeline before any function can continue. I believe this will cause the Machine IR for all functions to become resident in memory at once. This will substantially increase the memory usage of the compiler.

arsenm added a subscriber: arsenm.Mon, Mar 23, 1:11 PM
arsenm added inline comments.
llvm/lib/Target/X86/X86RetpolineThunks.cpp
164

s/unsigned/Register

213–215

This isn't the right way to do this, and the stated reasoning doesn't make sense. The pass should override getRequiredProperties/getSetProperties/getClearedProperties. This isn't a regalloc pass that actually triggers these conditions

This probably has a hidden side effect on the pass manager construction. Inserting a module pass into the middle of the pipeline effectively introduces a serialization point in the middle of the pipeline. This means all functions have to reach this point in the pipeline before any function can continue. I believe this will cause the Machine IR for all functions to become resident in memory at once. This will substantially increase the memory usage of the compiler.

Isn't this what already happens with the SymbolRewriter?

I had also tried grouping the X86RetpolineThunks ModulePass with some earlier module passes, but then later passes would attempt to perform transformations that assume SSA form. Is there a way to move the pass earlier in the pipeline, and prevent any subsequent passes from modifying the retpolines?

This probably has a hidden side effect on the pass manager construction. Inserting a module pass into the middle of the pipeline effectively introduces a serialization point in the middle of the pipeline. This means all functions have to reach this point in the pipeline before any function can continue. I believe this will cause the Machine IR for all functions to become resident in memory at once. This will substantially increase the memory usage of the compiler.

Isn't this what already happens with the SymbolRewriter?

Isn't SymbolRewriter before any of the MachineIR passes?

I had also tried grouping the X86RetpolineThunks ModulePass with some earlier module passes, but then later passes would attempt to perform transformations that assume SSA form. Is there a way to move the pass earlier in the pipeline, and prevent any subsequent passes from modifying the retpolines?

Are there other ModulePasses after isel?

zbrid added a comment.Mon, Mar 23, 2:13 PM

I don't see any compelling reason to refactor this pass as extensively as this. I would prefer a patch that's a non functional change while appropriately renames the pass and other things used by the retpoline thunks pass so future readers of the code know the pass emits non-retpoline thunks as well as retpolines. That would also mean you could apply the integrated LVI/retpoline patch from before almost as-is on top of that refactor.

I think the original pass works well enough as implemented for LVI/SESES purposes.

sconstab abandoned this revision.Wed, Mar 25, 4:39 PM

Due to performance/memory reasons, it is not a good idea to refactor this pass into a ModulePass.