Page MenuHomePhabricator

[RFC] Add MachineInstr::MIFlag parameter to storeRegToStackSlot
Needs ReviewPublic

Authored by asb on Feb 17 2017, 1:29 PM.

Details

Summary

This patch accompanies the RFC Setting MachineInstr flags through storeRegToStackSlot, see there for a full description of the problem and motivation.

This patch takes the approach of adding a MachineInstr::MIFlag parameter to storeRegToStackSlot. In its current state, it is meant to illustrate the proposal. A similar change should be made to loadRegFromStackSlot.

Diff Detail

Event Timeline

asb created this revision.Feb 17 2017, 1:29 PM
sdardis edited edge metadata.Feb 17 2017, 3:40 PM

This looks ok to me, you should probably wait for others to comment. One major point I can see is that the base TTI object should have the default argument of MachineInstr::NoFlags for Flags. This should be documented in the comments as the expected behaviour. The target derived TII objects will also need this. It should reduce the number of cases of

.setMIFlags(NoFlags);

appearing thought the codebase.

This may be a dumb question, but doesn't the call to setMIFlags(Flags) run the risk of overwriting any flags set on the instruction through other means? I assume that instructions defined by a back end in the target description files could set flags when the instruction is built which would then be overridden here. Wouldn't we want the flags to be added (or-ed) to any existing flags on the instruction?
Of course, I may be misunderstanding the mechanism by which the flags are set so this could very well not be an issue. Feel free to ignore the comment if that is so.

Also, I agree that the default argument for the Flags parameter does seem like a cleaner solution.

asb added a comment.EditedFeb 22 2017, 12:09 PM

@nemanjai: MachineInstr::setFlag will do Flags |= Flag, so any previously set flags are maintained.

I agree that having a NoFlags default would be better, I mainly left it as required to potentially benefit from more eyes on the storeRegToStackSlot call sites while the patch is in its early stages.

I'm starting to wonder if I'd be better off making it so emitPrologue and emitEpilogue take an iterator pointing to after the callee-saved spill and before the callee-saved restores, particularly as it sounds like it's not particularly important to have these flags on callee saves/restores (ref), beyond this task of finding where to set the frame pointer.

qcolombet edited edge metadata.Mar 7 2017, 9:28 AM

Hi Alex,

I'm starting to wonder if I'd be better off making it so emitPrologue and emitEpilogue take an iterator pointing to after the callee-saved spill and before the callee-saved restores, particularly as it sounds like it's not particularly important to have these flags on callee saves/restores (ref), beyond this task of finding where to set the frame pointer.

On top of that I'd say this is not clear, at least to me, that we need to mark those as frame setup. I start to believe that we only need to mark only the operations that do stack/frame pointer adjustments.

Could you dig a little more before moving forward?

Cheers,
-Quentin