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

Guozhi Wei via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Oct 21 08:41:25 PDT 2021


Carrot 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]]
----------------
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.


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

https://reviews.llvm.org/D111857



More information about the llvm-commits mailing list