Page MenuHomePhabricator

[GlobalISel] Change MachineIRBuilder's SrcOp to contain subregister info
AbandonedPublic

Authored by aemerson on Mar 15 2019, 5:48 PM.

Details

Summary

r356304 changed the buildCopy() call to allow passing in a subregister index as well as a source reg. However if the CSE builder learns to do CSE on copies then it can cause incorrect behaviour as the CSE lookups for {src-reg, sub-reg} pairs will only check for the src-reg, as the sub-reg is added after the buildInstr() call.

This change folds an optional subreg index into SrcOp, and teaches CSEInfo how deal with it.

Diff Detail

Event Timeline

aemerson created this revision.Mar 15 2019, 5:48 PM
arsenm added a subscriber: arsenm.Mar 15 2019, 6:18 PM

I would think we would want to avoid any subregister indexes until as late as possible. Does the verifier reject them for G_* instructions?

I would think we would want to avoid any subregister indexes until as late as possible. Does the verifier reject them for G_* instructions?

In practice yes we’d only really use this during selection, but if someone did want to use MachineIRBuilder for emitting copies (as we do in arm64), it should be safe w.r.t CSE as well.

In general I'm not too fond of building very target specific instructions involving subregs with the MachineIRBuilder (As these are only likely to be used in a few places in the selector). Specifically for this case of building a subreg, you can say

Builder.buildInstr(COPY, {Dst},{})
  .addReg(Src, 0, SubReg);

which is not too much larger than (and doesn't require making every SrcOp bigger).

Builder.buildInstr(COPY, {Dst}, {{Src, SubReg}});

While adding CSE support for COPYs with Subregs is great, I'm not sure how often it'll trigger and be useful (If it does occur then great). Also should we start adding support for building other kinds of target instructions - Adding immediate operands/target index nodes etc?
My approach for building non generic instructions have been with using the .addImm(..).addReg(...)... pattern - but if others strongly feel the need to add the ability to pass in all of the operands at once to the builder interfaces, and enabling CSE for them, then we can go ahead with this change.

aemerson abandoned this revision.Mar 18 2019, 11:32 AM

In general I'm not too fond of building very target specific instructions involving subregs with the MachineIRBuilder (As these are only likely to be used in a few places in the selector). Specifically for this case of building a subreg, you can say

Builder.buildInstr(COPY, {Dst},{})
  .addReg(Src, 0, SubReg);

which is not too much larger than (and doesn't require making every SrcOp bigger).

Builder.buildInstr(COPY, {Dst}, {{Src, SubReg}});

While adding CSE support for COPYs with Subregs is great, I'm not sure how often it'll trigger and be useful (If it does occur then great). Also should we start adding support for building other kinds of target instructions - Adding immediate operands/target index nodes etc?
My approach for building non generic instructions have been with using the .addImm(..).addReg(...)... pattern - but if others strongly feel the need to add the ability to pass in all of the operands at once to the builder interfaces, and enabling CSE for them, then we can go ahead with this change.

In that case I'll revert r356304 keeping in mind that we should take care not to enable COPY CSE in the selector in future, as it won't be subreg safe.