[PATCH] D111857: 【TwoAddressInstructionPass】 Put all new instructions into DistanceMap

[Pengfei Wang via Phabricator via llvm-commits]

pengfei added inline comments.


================
Comment at: llvm/test/CodeGen/X86/distancemap.mir:71
+    ; CHECK-NEXT: [[COPY6:%[0-9]+]]:gr16 = COPY [[COPY3]]
+    ; CHECK-NEXT: [[ADD16rr:%[0-9]+]]:gr16 = ADD16rr [[ADD16rr]], killed [[COPY4]], implicit-def $eflags
+    ; CHECK-NEXT: undef %11.sub_16bit:gr32 = COPY killed [[ADD16rr]]
----------------
Carrot wrote:
> pengfei wrote:
> > Carrot wrote:
> > > pengfei wrote:
> > > > This does seem correct to me. The input in line 88 is
> > > > `%10:gr16 = ADD16rr killed %3:gr16($di), killed %5:gr16, ...`
> > > > But the output turns into add `[[COPY4]]($esi)`?
> > > The operands are commuted. This is the exact difference caused by this patch.
> > > 
> > > Function isProfitableToCommute calls noUseAfterLastDef with argument [[COPY3]] when processing this ADD instruction, DistanceMap is used in noUseAfterLastDef to decide MI's position.  When the LEA instruction was generated, several COPY and IMPLICIT_DEF instructions are generated. If they are not added to DistanceMap, noUseAfterLastDef misses the use of [[COPY3]] and returns wrong value. With this patch noUseAfterLastDef can correctly find the use of [[COPY3]] and returns correct value, causes isProfitableToCommute to return different commute decision.
> > Oh, I was misled by the name `COPY6`. The VR names are nonsense here. This should be bug for update script.
> > Unfortunately, this test doesn't reveal the intent of the patch either. The swapped `COPY3` and `COPY4` doesn't show the necessity, which is the same as other tests. I'd suggest to remove it due to the misleading output.
> > Besides, both `COPY3` and `COPY4` are used in LEA. The explanation here doesn't persuasive.
> > I'd like to keep the code as is until we see a functional failure. But I'm neutral :)
> I have already commented in my previous reply that I can't find a test to show the problem in current code base, so this test just shows the difference, not a real problem.
> 
> But the usage of DistanceMap is not very complex. For every processed MI we insert it into DistanceMap, then we can easily find the position of an MI if it is already processed. In this case noUseAfterLastDef returns false for [[COPY3]] at the position of later ADD is obviously wrong since it is actually used in a COPY instruction. Luckily the wrong result of noUseAfterLastDef is only used to guide commuting, so it doesn't cause errors in generated code.
> 
> In my optimization, I also use DistanceMap to find if an MI has been processed. I want to change registers in unprocessed MIs. Because DistanceMap doesn't contains COPY instructions generated together with LEA, so my code thinks the COPY has not been processed and changed the COPY instruction, this time it causes wrong code generated.
> 
> So the conclusion is:
>    1. There is a logic error in the usage of DistanceMap, it is not difficult to point out by analyzing the source code.
>    2. We can see the difference of the bug fixing, but can't see anything wrong with this problem because it is only used to guide commuting in current code base. So you'll never see a functional failure.
>    3. In my code I use DistanceMap to find if an MI need to be changed, this time the bug can cause wrong code generated.
I didn't dig deep into the code, just some general questions:
1. The distances in DistanceMap seem to be the order of input. What's the mean with this change? I don't think you counted all new COPY MI. E.g. the one in line 56, right?
2. DistanceMap uses insert and erase methods in other places, do you need to do the same thing for the insert and remove the inserted COPYs together when erase?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111857/new/

https://reviews.llvm.org/D111857