Page MenuHomePhabricator

[TableGen][AsmMatcherEmitter] Generate assembler checks for tied operands

Authored by sdesmalen on Dec 20 2017, 6:40 AM.



This extends TableGen's AsmMatcherEmitter with code that generates
a table with tied-operand constraints. The constraints are checked
when parsing the instruction. If an operand is not equal to its tied operand,
the assembler will give an error.

Patch [2/3] in a series to add operand constraint checks for SVE's predicated ADD/SUB.

Diff Detail


Event Timeline

sdesmalen created this revision.Dec 20 2017, 6:40 AM
fhahn added a comment.Dec 21 2017, 7:33 AM

Thanks Sander, I left some comments.

This is needed for SVE, because some instructions force 2 registers to be equal for certain versions of the instruction. e.g.

add z1, z2, z3 # OK
add z1, p0/m, z2, z3 # Not OK, for predicated add the result reg must be equal to the first data src reg.

Adding Oliver as well, maybe you have some thoughts on the error handling in this case. Is there another way to handle this case?

531 ↗(On Diff #127709)

Do we need a set here? It seems like we could just used a SmallVector instead. It looks like a set is used to avoid adding duplicated constraints for an instruction, right?

But the tied constraint only gets added to one of the 2 operands (I think), see CodeGenInstructions::ParseConstraint.

Also, those constraints are referred to as tied elsewhere. AsmOperandTiedContraints would be a more consistent name I think

910 ↗(On Diff #127709)

Can AsmOperands be const?

914 ↗(On Diff #127709)

Why not AsmOperands.begin(), AsmOperands.end()?

919 ↗(On Diff #127709)

Why not AsmOperands.end()?

922 ↗(On Diff #127709)

Why not AsmOperands.begin()?

988 ↗(On Diff #127709)

For relatively short type names, using explicit types slightly improves readability here and elsewhere in this patch

1007 ↗(On Diff #127709)

Make type here size_t.

2911 ↗(On Diff #127709)

Please use Tied in the name here and throughout the function to make it clear that this only deals with Tied constraints.

2916 ↗(On Diff #127709)

Please use something more descriptive than DCL.

2934 ↗(On Diff #127709)

Use something more descriptive than x.

2957 ↗(On Diff #127709)

Having a struct for the SearchValue would better explain what the 3 fields are supposed to be.

fhahn added a comment.Dec 21 2017, 7:33 AM
This comment was removed by fhahn.
sdesmalen updated this revision to Diff 128426.Jan 2 2018, 9:10 AM

Replaced unordered set by SmallVector and did some refactoring to improve readability.

sdesmalen marked 11 inline comments as done.Jan 2 2018, 9:12 AM
sdesmalen added inline comments.
914 ↗(On Diff #127709)

I can't answer the 'why', but I did fix it in the latest iteration of this patch :)

fhahn added inline comments.Jan 3 2018, 6:21 AM
969 ↗(On Diff #128426)

I think the loop body here can be simplified by using CGIOp.getTiedRegister(); rather than checking the constraints here:

+    int TiedReg = CGIOp.getTiedRegister();
+    if (TiedReg == -1)
+      continue;
+    Optional<size_t> LHSIdx = getAsmOperandIdx(AsmOperands, CGIOp.Name);
+    Optional<size_t> RHSIdx = getAsmOperandIdx(AsmOperands, CGIOperands[TiedReg].Name);
+    // Skipping operands with constraint but no reference in the
+    // AsmString. No need to throw a warning, as it's normal to have
+    // a $dst operand in the outs dag that is constrained to a $src
+    // operand in the ins dag but that does not appear in the AsmString.
+    if (!LHSIdx || !RHSIdx)
+      continue;
+    // Add the constraint. Using min/max as we consider constraint
+    // pair {A,B} and {B,A} the same
+    size_t AddMnemonicIdx = HasMnemonicFirst;
+    AsmOperandTiedConstraints.emplace_back(
+        std::min(*LHSIdx, *RHSIdx) + AddMnemonicIdx,
+        std::max(*LHSIdx, *RHSIdx) + AddMnemonicIdx);
+  }
sdesmalen updated this revision to Diff 128527.Jan 3 2018, 8:35 AM
sdesmalen marked an inline comment as done.

Replaced a custom loop with call to CGIOp.getTiedRegister().

sdesmalen marked an inline comment as done.Jan 3 2018, 8:35 AM
sdesmalen added inline comments.
969 ↗(On Diff #128426)

Thanks Florian, I've implemented your suggestion.

fhahn accepted this revision.Jan 3 2018, 9:01 AM

Thanks Sander, LGTM, with 3 small comments. Please hold off committing this a couple of days, to give other people some more time to voice concerns.

2934 ↗(On Diff #128527)

Can you re-name this to checkAsmOperandTiedConstraints?

3639 ↗(On Diff #128527)

3 spaces here?

3721 ↗(On Diff #128527)

3 spaces here?

This revision is now accepted and ready to land.Jan 3 2018, 9:01 AM
sdesmalen updated this revision to Diff 128533.Jan 3 2018, 9:38 AM
sdesmalen marked an inline comment as done.
  • Renamed checkAsmOperandConstraints -> checkAsmTiedOperandConstraints.
  • Removed unnecessary indentation.
sdesmalen marked 3 inline comments as done.Jan 3 2018, 9:38 AM
sdesmalen closed this revision.Jan 10 2018, 2:12 AM
This revision was automatically updated to reflect the committed changes.