This is an archive of the discontinued LLVM Phabricator instance.

[TableGen][AsmMatcherEmitter] Remove boolean 'Hack' parameter
AbandonedPublic

Authored by sdesmalen on Jan 4 2018, 9:09 AM.

Details

Summary

An instruction alias should be able to specify the same
(tied) operand twice in the asm string if the asm parser makes
sure the tied operand constraint is satisfied.

Diff Detail

Event Timeline

apazos added a comment.Jan 5 2018, 3:48 PM

Thanks Daniel for pushing this patch.
I tested it internally with my test cases and it works fine.
I also built all targets and did a 'ninja check-all' and I did not detect any new failures.
So I think no target breaks if we remove this Hack flag. Did you confirm it?

Now with this issue fixed, I want to raise another related issue (with tied operands) in AsmWriterEmitter::EmitPrintAliasInstruction.

Note that code bails out if number of operands in the AsmString is higher than the number of operands in the ResultString:

// Don't emit the alias if it has more operands than what it's aliasing.
   if (NumResultOps < CountNumOperands(CGA.AsmString, Variant))
     continue;

So if you have:
Asmstring: "op $rd, $rd, $rs2 ->number of operands is 3
ResultString "newop $rd, $rs2" where rd is tied with the other rs1 input operand, the number of operands is 2.

I think here we should either count number of different operands in the AsmString or add the number of tied operands to the ResultString total number.

apazos added inline comments.Jan 5 2018, 6:08 PM
utils/TableGen/AsmMatcherEmitter.cpp
1121

change << "ignoring instruction with tied operand ' to << "instruction with tied operand ' or remove the warning...

I tested it internally with my test cases and it works fine.
I also built all targets and did a 'ninja check-all' and I did not detect any new failures.
So I think no target breaks if we remove this Hack flag. Did you confirm it?

We have specific tests for SVE for which this change is needed which all pass. Also, doing a check-all found no issues for other targets, so I think we should be good.

Now with this issue fixed, I want to raise another related issue (with tied operands) in AsmWriterEmitter::EmitPrintAliasInstruction.
Note that code bails out if number of operands in the AsmString is higher than the number of operands in the ResultString:

Are you saying that you have a case where you expect the Alias to have more operands than the actual instruction? Because I can't directly think of a reason why having a duplicate operand would be useful in an instruction alias?

Thanks Daniel for pushing this patch.

I'm not Daniel ;)

utils/TableGen/AsmMatcherEmitter.cpp
1121

I think the 'ignoring' is there in the message because the instruction is actually ignored (see the caller of validate()), which is actually a good thing to warn about.

apazos added a comment.Jan 8 2018, 3:45 PM

Hi Sander, sorry for the name confusion.
The callers of validate() skip the instruction in one case, but not in the other. So you are trying to say the message is for the case it is skipped.
What is your alias test case? In my case the AsmString has repeated operands, but the result instruction it maps to is defined with tied operands.
Anyways, this change looks fine for me, if you want to move to validate/merge.

fhahn requested changes to this revision.Jan 10 2018, 6:28 AM

I do not think just removing the error message for the hack is the right thing to do.

With this patch, it is possible to define instruction aliases like (AArch64 example)

def : InstAlias<"mov $dst, $src, $src, $src", (ORRWrs GPR32:$dst, WZR, GPR32:$src, 0), 2>;

And now

bar:
        mov w1, w0, w2, w3
        mov w1, w0, w0, w0

gets encoded to the following by llvm-mc:

bar:
	orr	w1, wzr, w0             // encoding: [0xe1,0x03,0x00,0x2a]
	orr	w1, wzr, w0             // encoding: [0xe1,0x03,0x00,0x2a]

If we want to support instruction aliases with tied operands, it seems like we have to add tied constraints for strings with repeated operands automatically or provide a way to specify the alias as "mov $dst, $src1, $src2, $src3" with "$src1 = $src2 = $src3"

This revision now requires changes to proceed.Jan 10 2018, 6:28 AM
fhahn added inline comments.Jan 10 2018, 6:54 AM
utils/TableGen/AsmMatcherEmitter.cpp
1119

This warning will only be shown if tablegen is run with -debug, which is not the case for normal debug builds (AFAIK)

Hi Florian, I think when you define an alias with repeated operands you need to additionally define checkEarlyTargetMatchPredicate in your target to make sure that a if you are trying to build an "orr" instruction coming from a "mov" instruction, the mov instruction coming in has the right tied operands. Otherwise you reject the match. This warning causes failure in an build with asserts enabled.

sdesmalen abandoned this revision.Jan 19 2018, 5:45 AM

I'm abandoning this change as I have a new patch that addresses this properly: D42293.

In short, I think the error for aliases can be removed if and only if the repeated operand actually maps to a 'tied' operand that is checked during assembling.
So, for the example that @fhahn gave:

def : InstAlias<"mov $dst, $src, $src, $src", (ORRWrs GPR32:$dst, WZR, GPR32:$src, 0), 2>;

The second $src is not linked to a tied operand, so repeating $src should cause TableGen to give an error. To demonstrate this, I added another SVE patch that defines an alias with a repeated operand here: D42295.
Adding another reference to $Zdn in the InstAlias asm string would also cause an error by TableGen.

While investigating this, I found that the generic tied operands constraint checks as implemented in D41446 did not properly cover aliases, this new patch fixes that.
@apazos, note that these constraint checks are a generalization of 'checkEarlyTargetMatchPredicate' which only seems to be implemented by Mips, so I think that can be removed when D42293 is merged.

fhahn added a comment.Jan 19 2018, 6:09 AM

Thanks Sander! I'll have a look over the weekend