This is an archive of the discontinued LLVM Phabricator instance.

Fix endianness bug in DAGCombiner::visitTRUNCATE and visitEXTRACT_VECTOR_ELT
ClosedPublic

Authored by fpichet on Jul 4 2017, 11:13 AM.

Details

Summary

Do not assume little endian architecture in DAGCombiner::visitTRUNCATE and DAGCombiner::visitEXTRACT_VECTOR_ELT.
PR33682

Diff Detail

Event Timeline

fpichet created this revision.Jul 4 2017, 11:13 AM
sdardis edited reviewers, added: sdardis; removed: llvm-commits.Jul 5 2017, 10:26 AM
sdardis added a subscriber: sdardis.

You should upload patches with more context i.e. git diff -U9999.

I'm seeing the reported issue with MIPS, here's a testcase, you may need to file the rough edges off.

It can go in test/CodeGen/Mips/pr33682.ll

; RUN: llc -march=mips -mcpu=mips32  < %s | FileCheck --check-prefixes=ALL,BE
; RUN: llc -march=mipsel -mcpu=mips32  < %s | FileCheck --check-prefixes=ALL,LE
  
; Verify visitTRUNCATE respects endianness when transforming truncates to extract vector element.

; ALL-LABEL: a:
; BE: lw $2, 4($4)
; LE: lw $2, 0($4) 
  
define i32 @a(<2 x i32> * %a) {
entry:
  %0 = load <2 x i32>, <2 x i32> * %a
  %1 = bitcast <2 x i32> %0 to i64
  %2 = trunc i64 %1 to i32
  ret i32 %2
}
fpichet updated this revision to Diff 105367.Jul 5 2017, 8:00 PM

Added test case

RKSimon added a subscriber: RKSimon.
RKSimon added inline comments.
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
8377

I'm not sure this is generally correct - for example what about the (i16 trunc (i64 (bitcast v4i16:x))) case? Shouldn't the extracted big-endian index then be 3?

fpichet updated this revision to Diff 105995.Jul 11 2017, 4:02 AM

Update patch taking into account vector size > 2
Also reuse existing isLE variable.

RKSimon edited edge metadata.Jul 11 2017, 4:36 AM

Cheers, does anyone have any further comments?

sdardis edited edge metadata.Jul 11 2017, 4:40 AM

2 quick things, summary could do with an update (as now there is a test case), but that can be done on commit.

The other is that the case you've highlighted @RKSimon should probably be added to the test case.

Thanks.

Waiting on the additional bug mentioned in PR33682

fpichet edited the summary of this revision. (Show Details)Jul 11 2017, 1:29 PM
This comment was removed by fpichet.
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
8377

I agree, I'll update the patch.

fpichet updated this revision to Diff 106137.Jul 11 2017, 7:47 PM
fpichet retitled this revision from Fix endianness bug in DAGCombiner::visitTRUNCATE to Fix endianness bug in DAGCombiner::visitTRUNCATE and visitEXTRACT_VECTOR_ELT.
fpichet edited the summary of this revision. (Show Details)

Update patch with fix for endianness bug in visitEXTRACT_VECTOR_ELT.

is it ok to commit now?

sdardis accepted this revision.Jul 24 2017, 2:25 AM

LGTM. @RKSimon No more comments from me.

This revision is now accepted and ready to land.Jul 24 2017, 2:25 AM
RKSimon accepted this revision.Jul 24 2017, 4:19 AM

LGTM as well

This revision was automatically updated to reflect the committed changes.